Bug 202356 – Add some assertions to convertUTF8ToUTF16().

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

<rdar://problem/52846813>

[Mark Lam]

Created attachment 379820 [details]
proposed patch.

[Saam Barati]

Comment on attachment 379820 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=379820&action=review

> Source/WTF/wtf/unicode/UTF8Conversion.cpp:95
> +    RELEASE_ASSERT(targetEnd - target <= std::numeric_limits<int>::max());

why does the code require this?

[Mark Lam]

Comment on attachment 379820 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=379820&action=review

Thanks for the review.

>> Source/WTF/wtf/unicode/UTF8Conversion.cpp:95
>> +    RELEASE_ASSERT(targetEnd - target <= std::numeric_limits<int>::max());
> 
> why does the code require this?

For the same reason as why there's a "RELEASE_ASSERT(sourceEnd - source <= std::numeric_limits<int>::max())" above: to validate that the client didn't pass in a targetEnd that is at a lower address than the targetStart.

[Mark Lam]

Surprisingly, looks like these assertions are failing on some tests.  I'm investigating.

[Mark Lam]

(In reply to Mark Lam from comment #4)
> Surprisingly, looks like these assertions are failing on some tests.  I'm
> investigating.

Turns out, it's too aggressive to assert that (targetEnd - target <= std::numeric_limits<int>::max()). Some clients actually did the work to calculate the exact number of UChars needed, not just the upper bound.  We should not penalize these clients.  I'll upload an updated patch with a better assertion.

[Mark Lam]

Created attachment 379821 [details]
proposed patch.

Let's try this on the EWS first.

[Mark Lam]

Comment on attachment 379821 [details]
proposed patch.

Thanks for the review.  Landing now.

[WebKit Commit Bot]

Comment on attachment 379821 [details]
proposed patch.

Clearing flags on attachment: 379821

Committed r250520: <https://trac.webkit.org/changeset/250520>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

(In reply to Mark Lam from comment #3)
> Comment on attachment 379820 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379820&action=review
> 
> Thanks for the review.
> 
> >> Source/WTF/wtf/unicode/UTF8Conversion.cpp:95
> >> +    RELEASE_ASSERT(targetEnd - target <= std::numeric_limits<int>::max());
> > 
> > why does the code require this?
> 
> For the same reason as why there's a "RELEASE_ASSERT(sourceEnd - source <=
> std::numeric_limits<int>::max())" above: to validate that the client didn't
> pass in a targetEnd that is at a lower address than the targetStart.

This isn't quite right. You were halving the output rang.

[Mark Lam]

(In reply to Saam Barati from comment #10)
> (In reply to Mark Lam from comment #3)
> > >> Source/WTF/wtf/unicode/UTF8Conversion.cpp:95
> > >> +    RELEASE_ASSERT(targetEnd - target <= std::numeric_limits<int>::max());
> > > 
> > > why does the code require this?
> > 
> > For the same reason as why there's a "RELEASE_ASSERT(sourceEnd - source <=
> > std::numeric_limits<int>::max())" above: to validate that the client didn't
> > pass in a targetEnd that is at a lower address than the targetStart.
> 
> This isn't quite right. You were halving the output rang.

I clarified this with Saam offline: I am not halving the output range because target and targetEnd are in units of UChar*.  Hence, this assertion is correct.