Summary: | Improve assertions in StringParsingBuffer | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||||
Component: | New Bugs | Assignee: | Sam Weinig <sam> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, darin, ews-watchlist, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 213460 | ||||||||||||
Attachments: |
|
Description
Sam Weinig
2020-06-25 18:14:22 PDT
Created attachment 402838 [details]
Patch
Created attachment 402841 [details]
Patch
Comment on attachment 402841 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402841&action=review > Source/WTF/wtf/text/StringParsingBuffer.h:55 > + ASSERT(static_cast<std::ptrdiff_t>(end - characters) < std::numeric_limits<unsigned>::max()); Surprised that the static_cast on the left side is necessary. Surprised that a static_cast on the right side isn’t necessary to ensure we don’t get signed/unsigned comparison warnings. > Source/WTF/wtf/text/StringParsingBuffer.h:86 > - constexpr void advanceBy(unsigned places) > + constexpr void advanceBy(size_t places) Not sure how I feel about this even though I suggested it. In general if we pass too large a type it will get chopped off. But if the value is too big for unsigned the code won’t work anyway. Not sure why this is more important than taking size_t in operator[]. Could imagine setting this to a super-large integer type and doing a RELEASE_ASSERT. Maybe we should just be leaving this unsigned? Issues don’t seem different here than in operator[] or the constructor. Created attachment 402907 [details]
Patch
Comment on attachment 402907 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402907&action=review > Source/WTF/wtf/text/StringParsingBuffer.h:55 > + ASSERT(end - characters < std::numeric_limits<unsigned>::max()); <= ? Created attachment 402956 [details]
Patch
Committed r263620: <https://trac.webkit.org/changeset/263620> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402956 [details]. |