Crashes in setTextForIterator

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.

Created attachment 221864 [details]
Patch

[Alexey Proskuryakov]

I'm wondering if this is the same as bug 127408.

[Brent Fulgham]

Comment on attachment 221864 [details]
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?

(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.

[Brent Fulgham]

(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;

(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.

[Brent Fulgham]

(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?

[Brent Fulgham]

Comment on attachment 221864 [details]
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.

[Brent Fulgham]

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!

(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.

Created attachment 221875 [details]
Patch

(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 :)

[Brent Fulgham]

Comment on attachment 221875 [details]
Patch

Looks great! r=me

[WebKit Commit Bot]

Comment on attachment 221875 [details]
Patch

Clearing flags on attachment: 221875

Committed r162544: <http://trac.webkit.org/changeset/162544>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

*** Bug 127408 has been marked as a duplicate of this bug. ***