The caseConvert function should not truncate strings with zero length.

The caseConvert function should not truncate strings with zero length.

The truncateAssumingIsolated function is only called for non-empty
strings in caseConvert. The type of targetLength is changed to
unsigned from signed, which can cause buffer overflows with
specially constructed strings.

BUG=425478
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186016

[zherczeg, 2014-11-18 07:32:00]

zherczeg.u-szeged@partner.samsung.com changed reviewers:
+ aandrey+blink@chromium.org, mikhailt@google.com

[zherczeg, 2014-11-19 09:51:50]

zherczeg.u-szeged@partner.samsung.com changed reviewers:
+ eseidel@chromium.org, morrita@chromium.org, tkent@chromium.org
- aandrey+blink@chromium.org

[zherczeg, 2014-11-19 09:52:11]

On 2014/11/19 09:51:50, zherczeg wrote:

Please review this patch.

[gmorrita, 2014-11-19 18:10:23]

morrita@google.com changed reviewers:
+ morrita@google.com

[gmorrita, 2014-11-19 18:10:23]

lgtm.

[gmorrita, 2014-11-19 18:10:36]

The CQ bit was checked by morrita@google.com

[commit-bot: I haz the power, 2014-11-19 18:11:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/729173003/1

[commit-bot: I haz the power, 2014-11-19 18:24:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-19 18:24:03]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)

[tkent, 2014-11-20 02:34:16]

I don't think Patch Set 1 is a right fix.  It breaks assumption of
truncateAssumingIsolated().  We should not call it if the string is not
isolated.
It is called by caseConvert(), right? Probably we should add empty check to
caseConvert().

[zherczeg, 2014-11-20 11:38:51]

On 2014/11/20 02:34:16, tkent wrote:
> It breaks assumption of
> truncateAssumingIsolated().  We should not call it if the string is not
> isolated.

You are totally right! I am sorry I usually notice such things.

I also noticed however that the type of targetLength is signed (the compiler
complained about the new assert). Since every other function expects unsigned
values, this could cause buffer overflow (security) issues.

[zherczeg, 2014-11-20 11:52:27]

> I also noticed however that the type of targetLength is signed (the compiler
> complained about the new assert). Since every other function expects unsigned
> values, this could cause buffer overflow (security) issues.

I mean if length is 0x80000000, and the string contains 'a' letters:

int32_t targetLength = length; // unsigned truncate, same value
output->truncateAssumingIsolated(targetLength); // signed conversion to
0xffffffff80000000

From this point, everybody thinks the buffer is much bigger than it really is.

[zherczeg, 2014-11-24 07:47:43]

tkent, could you review the new patch?

[tkent, 2014-11-25 00:41:22]

https://codereview.chromium.org/729173003/diff/20001/Source/wtf/text/StringIm...
File Source/wtf/text/StringImpl.cpp (right):

https://codereview.chromium.org/729173003/diff/20001/Source/wtf/text/StringIm...
Source/wtf/text/StringImpl.cpp:698: ASSERT(targetLength <= length);
This assertion is not correct.

input: ß  (length=1)
converter: u_strToUpper
output: SS (targetLength=2)

[zherczeg, 2014-11-25 08:01:47]

>
https://codereview.chromium.org/729173003/diff/20001/Source/wtf/text/StringIm...
> Source/wtf/text/StringImpl.cpp:698: ASSERT(targetLength <= length);
> This assertion is not correct.

I presume U_BUFFER_OVERFLOW_ERROR is returned when targetLength > length. And
this ASSERT is just the duplication of the ASSERT in truncateAssumingIsolated,
but I think it is useful to have one here as well, especially when length == 0.
But if you wish I can remove it of course.

[tkent, 2014-11-25 08:20:02]

On 2014/11/25 08:01:47, zherczeg wrote:
> >
>
https://codereview.chromium.org/729173003/diff/20001/Source/wtf/text/StringIm...
> > Source/wtf/text/StringImpl.cpp:698: ASSERT(targetLength <= length);
> > This assertion is not correct.
> 
> I presume U_BUFFER_OVERFLOW_ERROR is returned when targetLength > length. 

After U_BUFFER_OVERFLOW_ERROR, we have the the second call to converter(), and
it will succeed.

> And
> this ASSERT is just the duplication of the ASSERT in truncateAssumingIsolated,

No, the ASSERT in truncateAssumingIsolated is different.  It's comparing
output->m_length and targetLength.  Your assertion is comparing source length
and targetLength.

[zherczeg, 2014-11-26 07:57:29]

> No, the ASSERT in truncateAssumingIsolated is different.  It's comparing
> output->m_length and targetLength.  Your assertion is comparing source length
> and targetLength.

True. Ok, ASSERT is removed, description is updated, patch uploaded. Anything
else?

[tkent, 2014-11-26 07:58:44]

The CQ bit was checked by tkent@chromium.org

[tkent, 2014-11-26 07:58:44]

lgtm

[commit-bot: I haz the power, 2014-11-26 07:59:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/729173003/40001

[commit-bot: I haz the power, 2014-11-26 09:22:32]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186016