127424 2014-01-22 06:19:56 -0800 Crashes in setTextForIterator 2014-01-22 13:02:39 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 peavo webkit-unassigned ap bfulgham commit-queue sam oldest_to_newest 970679 0 peavo 2014-01-22 06:19:56 -0800 I'm getting a crash in the function setTextForIterator in WebCore/platform/text/TextBreakIterator.cpp. We need to reserve sufficient memory for the mem set in the function openLatin1UTextProvider in WebCore/platform/text/icu/UTextProviderLatin1.cpp. 970681 1 221864 peavo 2014-01-22 06:24:37 -0800 Created attachment 221864 Patch 970720 2 ap 2014-01-22 09:47:32 -0800 I'm wondering if this is the same as bug 127408. 970733 3 221864 bfulgham 2014-01-22 10:07:32 -0800 Comment on attachment 221864 Patch I'd like better understanding of why we are off by one here. Are some routines expecting the buffer to hold a null termination? I notice that most of the uses of UTextWIthBufferInlineCapacity are actually "UTextWithBufferInlineCapacity + 1" (for memsets, etc.) Is that where the crash was occurring? 970741 4 peavo 2014-01-22 10:13:19 -0800 (In reply to comment #3) > (From update of attachment 221864 [details]) > I'd like better understanding of why we are off by one here. Are some routines expecting the buffer to hold a null termination? I notice that most of the uses of UTextWIthBufferInlineCapacity are actually "UTextWithBufferInlineCapacity + 1" (for memsets, etc.) Is that where the crash was occurring? No, the crash occurs at the end of setTextForIterator, because runtime checks detects that we have written past a stack variable (UTextWithBuffer textLocal, in setTextForIterator), and damaged the stack. It is the memset that writes past the stack variable. 970745 5 bfulgham 2014-01-22 10:19:33 -0800 (In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 221864 [details] [details]) > > I'd like better understanding of why we are off by one here. Are some routines expecting the buffer to hold a null termination? I notice that most of the uses of UTextWIthBufferInlineCapacity are actually "UTextWithBufferInlineCapacity + 1" (for memsets, etc.) Is that where the crash was occurring? > > No, the crash occurs at the end of setTextForIterator, because runtime checks detects that we have written past a stack variable (UTextWithBuffer textLocal, in setTextForIterator), and damaged the stack. > It is the memset that writes past the stack variable. I see. The call to "openLatin1UTextProvider" has the memset, and the memset always writes UTextWithBufferInlineCapacity + 1. It's unclear if we should be resizing the buffer, or revising the uses of UTextWithBufferInlineCapacity to avoid the additional "+ 1". For example, if we resize the buffer by one, is this math now wrong? (see UTextProviderLatin1.cpp line 125): uText->chunkNativeLimit = uText->chunkNativeStart + UTextWithBufferInlineCapacity; 970746 6 peavo 2014-01-22 10:24:44 -0800 (In reply to comment #5) > (In reply to comment #4) > > It's unclear if we should be resizing the buffer, or revising the uses of UTextWithBufferInlineCapacity to avoid the additional "+ 1". > Good point, that might be the case. > For example, if we resize the buffer by one, is this math now wrong? (see UTextProviderLatin1.cpp line 125): > > uText->chunkNativeLimit = uText->chunkNativeStart + UTextWithBufferInlineCapacity; I don't think this patch changes any logic, as we only allocate more space, without changing the UTextWithBufferInlineCapacity constant. 970750 7 bfulgham 2014-01-22 10:29:19 -0800 (In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > > It's unclear if we should be resizing the buffer, or revising the uses of UTextWithBufferInlineCapacity to avoid the additional "+ 1". > > > > Good point, that might be the case. > > > For example, if we resize the buffer by one, is this math now wrong? (see UTextProviderLatin1.cpp line 125): > > > > uText->chunkNativeLimit = uText->chunkNativeStart + UTextWithBufferInlineCapacity; > > I don't think this patch changes any logic, as we only allocate more space, without changing the UTextWithBufferInlineCapacity constant. I probably used a bad example. Consider Line 135 in the same file: uText->chunkNativeStart = uText->chunkNativeLimit - UTextWithBufferInlineCapacity; Now we position are starting point in the final element of the buffer, rather than one past it. Doesn't that mean we will get improper results? 970752 8 221864 bfulgham 2014-01-22 10:36:13 -0800 Comment on attachment 221864 Patch Looking through the code, there are numerous places where sizeof(buffer) is used, and others where UTextWithBufferInlineCapacity is used. I think the right fix is to change all the cases of "UTextWithBufferInlineCapacity + 1" to just "UTextWithBufferInlineCapacity". Otherwise, I am concerned that our iterator math will be wrong in some cases resulting in undefined behavior. The only concern I have with my suggestion is that there are uses where the UTextWithBuffer client assumes that the "UTextWithBufferInlineCapacity" is the number of valid characters, with an implicit extra null "byte" at the end. 970756 9 bfulgham 2014-01-22 10:39:17 -0800 Thank you for tracking down the cause of this problem. After talking your patch over with a couple of the other people on the team, I *think* the correct solution is to remove the "+1" from the various places where UTextWithBufferInlineCapacity is used. Note also that this method: static UText* uTextLatin1Clone(UText* destination, const UText* source, UBool deep, UErrorCode* status) calls utext_setup with a +1, unlike all other uses of utext_setup in the code base. Looks like you found a nasty little off-by-one we've been living with for some time! 970760 10 peavo 2014-01-22 10:47:40 -0800 (In reply to comment #9) > Thank you for tracking down the cause of this problem. After talking your patch over with a couple of the other people on the team, I *think* the correct solution is to remove the "+1" from the various places where UTextWithBufferInlineCapacity is used. > > Note also that this method: > static UText* uTextLatin1Clone(UText* destination, const UText* source, UBool deep, UErrorCode* status) > > calls utext_setup with a +1, unlike all other uses of utext_setup in the code base. > > Looks like you found a nasty little off-by-one we've been living with for some time! Thanks for looking into this :) Will update the patch. 970769 11 221875 peavo 2014-01-22 11:00:17 -0800 Created attachment 221875 Patch 970774 12 peavo 2014-01-22 11:06:25 -0800 (In reply to comment #8) > I think the right fix is to change all the cases of "UTextWithBufferInlineCapacity + 1" to just "UTextWithBufferInlineCapacity". Otherwise, I am concerned that our iterator math will be wrong in some cases resulting in undefined behavior. Updated the patch as you suggested, works nicely :) 970794 13 221875 bfulgham 2014-01-22 11:22:57 -0800 Comment on attachment 221875 Patch Looks great! r=me 970807 14 221875 commit-queue 2014-01-22 11:50:46 -0800 Comment on attachment 221875 Patch Clearing flags on attachment: 221875 Committed r162544: <http://trac.webkit.org/changeset/162544> 970808 15 commit-queue 2014-01-22 11:50:49 -0800 All reviewed patches have been landed. Closing bug. 970840 16 ap 2014-01-22 13:02:39 -0800 *** Bug 127408 has been marked as a duplicate of this bug. *** 221864 2014-01-22 06:24:37 -0800 2014-01-22 11:00:09 -0800 Patch bug-127424-20140122152424.patch text/plain 1296 peavo SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE2MjUxNikKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE0IEBACisyMDE0LTAxLTIyICBwZWF2b0Bv dXRsb29rLmNvbSAgPHBlYXZvQG91dGxvb2suY29tPgorCisgICAgICAgIENyYXNoIHdoZW4gcmVu ZGVyaW5nIHBhZ2UuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0xMjc0MjQKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICBXZSBuZWVkIHRvIHJlc2VydmUgc3VmZmljaWVudCBtZW1vcnkgZm9yIHRoZSBtZW0gc2V0 IGluIHRoZSBmdW5jdGlvbiBvcGVuTGF0aW4xVVRleHRQcm92aWRlciBpbiBVVGV4dFByb3ZpZGVy TGF0aW4xLmNwcC4KKworICAgICAgICAqIHBsYXRmb3JtL3RleHQvaWN1L1VUZXh0UHJvdmlkZXJM YXRpbjEuaDoKKwogMjAxNC0wMS0yMiAgQW50dGkgS29pdmlzdG8gIDxhbnR0aUBhcHBsZS5jb20+ CiAKICAgICAgICAgVXBkYXRlIG92ZXJsYXkgc2Nyb2xsYmFycyBpbiBzaW5nbGUgcGFzcwpJbmRl eDogU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vdGV4dC9pY3UvVVRleHRQcm92aWRlckxhdGluMS5o Cj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL3RleHQvaWN1L1VUZXh0UHJv dmlkZXJMYXRpbjEuaAkocmV2aXNpb24gMTYyNTE2KQorKysgU291cmNlL1dlYkNvcmUvcGxhdGZv cm0vdGV4dC9pY3UvVVRleHRQcm92aWRlckxhdGluMS5oCSh3b3JraW5nIGNvcHkpCkBAIC0zNSw3 ICszNSw3IEBAIGNvbnN0IGludCBVVGV4dFdpdGhCdWZmZXJJbmxpbmVDYXBhY2l0eQogCiBzdHJ1 Y3QgVVRleHRXaXRoQnVmZmVyIHsKICAgICBVVGV4dCB0ZXh0OwotICAgIFVDaGFyIGJ1ZmZlcltV VGV4dFdpdGhCdWZmZXJJbmxpbmVDYXBhY2l0eV07CisgICAgVUNoYXIgYnVmZmVyW1VUZXh0V2l0 aEJ1ZmZlcklubGluZUNhcGFjaXR5ICsgMV07CiB9OwogCiBVVGV4dCogb3BlbkxhdGluMVVUZXh0 UHJvdmlkZXIoVVRleHRXaXRoQnVmZmVyKiB1dFdpdGhCdWZmZXIsIGNvbnN0IExDaGFyKiBzdHJp bmcsIHVuc2lnbmVkIGxlbmd0aCwgVUVycm9yQ29kZSogc3RhdHVzKTsK 221875 2014-01-22 11:00:17 -0800 2014-01-22 11:50:46 -0800 Patch bug-127424-20140122200005.patch text/plain 2748 peavo SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE2MjUzNCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE1IEBACisyMDE0LTAxLTIyICBwZWF2b0Bv dXRsb29rLmNvbSAgPHBlYXZvQG91dGxvb2suY29tPgorCisgICAgICAgIENyYXNoZXMgaW4gc2V0 VGV4dEZvckl0ZXJhdG9yCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVn LmNnaT9pZD0xMjc0MjQKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKwor ICAgICAgICAqIHBsYXRmb3JtL3RleHQvaWN1L1VUZXh0UHJvdmlkZXJMYXRpbjEuY3BwOgorICAg ICAgICAoV2ViQ29yZTo6dVRleHRMYXRpbjFDbG9uZSk6IFByb3ZpZGUgY29ycmVjdCBidWZmZXIg c2l6ZSBpbiB1dGV4dF9zZXR1cCBmdW5jdGlvbiBjYWxsLgorICAgICAgICAoV2ViQ29yZTo6dVRl eHRMYXRpbjFBY2Nlc3MpOiBHaXZlIGNvcnJlY3QgYnVmZmVyIHNpemUgdG8gbWVtc2V0IGNhbGwu CisgICAgICAgIChXZWJDb3JlOjpvcGVuTGF0aW4xVVRleHRQcm92aWRlcik6IERpdHRvLgorCiAy MDE0LTAxLTIxICBKb3NlcGggUGVjb3Jhcm8gIDxwZWNvcmFyb0BhcHBsZS5jb20+CiAKICAgICAg ICAgV2ViIEluc3BlY3RvcjogUmVtb3ZlIHJlY29tcGlsZUFsbEpTRnVuY3Rpb25zIHRpbWVyIGlu IFNjcmlwdERlYnVnU2VydmVyCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS90ZXh0L2lj dS9VVGV4dFByb3ZpZGVyTGF0aW4xLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9w bGF0Zm9ybS90ZXh0L2ljdS9VVGV4dFByb3ZpZGVyTGF0aW4xLmNwcAkocmV2aXNpb24gMTYyNTE2 KQorKysgU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vdGV4dC9pY3UvVVRleHRQcm92aWRlckxhdGlu MS5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTY3LDcgKzY3LDcgQEAgc3RhdGljIFVUZXh0KiB1VGV4 dExhdGluMUNsb25lKFVUZXh0KiBkZQogICAgIGlmIChVX0ZBSUxVUkUoKnN0YXR1cykpCiAgICAg ICAgIHJldHVybiAwOwogCi0gICAgVVRleHQqIHJlc3VsdCA9IHV0ZXh0X3NldHVwKGRlc3RpbmF0 aW9uLCBzaXplb2YoVUNoYXIpICogKFVUZXh0V2l0aEJ1ZmZlcklubGluZUNhcGFjaXR5ICsgMSks IHN0YXR1cyk7CisgICAgVVRleHQqIHJlc3VsdCA9IHV0ZXh0X3NldHVwKGRlc3RpbmF0aW9uLCBz aXplb2YoVUNoYXIpICogVVRleHRXaXRoQnVmZmVySW5saW5lQ2FwYWNpdHksIHN0YXR1cyk7CiAg ICAgaWYgKFVfRkFJTFVSRSgqc3RhdHVzKSkKICAgICAgICAgcmV0dXJuIGRlc3RpbmF0aW9uOwog ICAgIApAQCAtODIsNyArODIsNyBAQCBzdGF0aWMgVVRleHQqIHVUZXh0TGF0aW4xQ2xvbmUoVVRl eHQqIGRlCiAgICAgcmVzdWx0LT5hID0gc291cmNlLT5hOwogICAgIHJlc3VsdC0+cEZ1bmNzID0g JnVUZXh0TGF0aW4xRnVuY3M7CiAgICAgcmVzdWx0LT5jaHVua0NvbnRlbnRzID0gKFVDaGFyKily ZXN1bHQtPnBFeHRyYTsKLSAgICBtZW1zZXQoY29uc3RfY2FzdDxVQ2hhcio+KHJlc3VsdC0+Y2h1 bmtDb250ZW50cyksIDAsIHNpemVvZihVQ2hhcikgKiAoVVRleHRXaXRoQnVmZmVySW5saW5lQ2Fw YWNpdHkgKyAxKSk7CisgICAgbWVtc2V0KGNvbnN0X2Nhc3Q8VUNoYXIqPihyZXN1bHQtPmNodW5r Q29udGVudHMpLCAwLCBzaXplb2YoVUNoYXIpICogVVRleHRXaXRoQnVmZmVySW5saW5lQ2FwYWNp dHkpOwogCiAgICAgcmV0dXJuIHJlc3VsdDsKIH0KQEAgLTEzMiw3ICsxMzIsNyBAQCBzdGF0aWMg VUJvb2wgdVRleHRMYXRpbjFBY2Nlc3MoVVRleHQqIHVUCiAgICAgICAgIGlmICh1VGV4dC0+Y2h1 bmtOYXRpdmVMaW1pdCA+IGxlbmd0aCkKICAgICAgICAgICAgIHVUZXh0LT5jaHVua05hdGl2ZUxp bWl0ID0gbGVuZ3RoOwogCi0gICAgICAgIHVUZXh0LT5jaHVua05hdGl2ZVN0YXJ0ID0gdVRleHQt PmNodW5rTmF0aXZlTGltaXQgLSAgVVRleHRXaXRoQnVmZmVySW5saW5lQ2FwYWNpdHk7CisgICAg ICAgIHVUZXh0LT5jaHVua05hdGl2ZVN0YXJ0ID0gdVRleHQtPmNodW5rTmF0aXZlTGltaXQgLSBV VGV4dFdpdGhCdWZmZXJJbmxpbmVDYXBhY2l0eTsKICAgICAgICAgaWYgKHVUZXh0LT5jaHVua05h dGl2ZVN0YXJ0IDwgMCkKICAgICAgICAgICAgIHVUZXh0LT5jaHVua05hdGl2ZVN0YXJ0ID0gMDsK IApAQCAtMjI4LDcgKzIyOCw3IEBAIFVUZXh0KiBvcGVuTGF0aW4xVVRleHRQcm92aWRlcihVVGV4 dFdpdGgKICAgICB0ZXh0LT5hID0gbGVuZ3RoOwogICAgIHRleHQtPnBGdW5jcyA9ICZ1VGV4dExh dGluMUZ1bmNzOwogICAgIHRleHQtPmNodW5rQ29udGVudHMgPSAoVUNoYXIqKXRleHQtPnBFeHRy YTsKLSAgICBtZW1zZXQoY29uc3RfY2FzdDxVQ2hhcio+KHRleHQtPmNodW5rQ29udGVudHMpLCAw LCBzaXplb2YoVUNoYXIpICogKFVUZXh0V2l0aEJ1ZmZlcklubGluZUNhcGFjaXR5ICsgMSkpOwor ICAgIG1lbXNldChjb25zdF9jYXN0PFVDaGFyKj4odGV4dC0+Y2h1bmtDb250ZW50cyksIDAsIHNp emVvZihVQ2hhcikgKiBVVGV4dFdpdGhCdWZmZXJJbmxpbmVDYXBhY2l0eSk7CiAKICAgICByZXR1 cm4gdGV4dDsKIH0K