<rdar://problem/52846813>
Created attachment 379820 [details] proposed patch.
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?
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.
Surprisingly, looks like these assertions are failing on some tests. I'm investigating.
(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.
Created attachment 379821 [details] proposed patch. Let's try this on the EWS first.
Comment on attachment 379821 [details] proposed patch. Thanks for the review. Landing now.
Comment on attachment 379821 [details] proposed patch. Clearing flags on attachment: 379821 Committed r250520: <https://trac.webkit.org/changeset/250520>
All reviewed patches have been landed. Closing bug.
(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.
(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.