Bug 213633 - Improve assertions in StringParsingBuffer
Summary: Improve assertions in StringParsingBuffer
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
Keywords: InRadar
Depends on:
Blocks: 213460
  Show dependency treegraph
Reported: 2020-06-25 18:14 PDT by Sam Weinig
Modified: 2020-06-27 09:08 PDT (History)
6 users (show)

See Also:

Patch (2.06 KB, patch)
2020-06-25 18:15 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (2.06 KB, patch)
2020-06-25 18:51 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (1.93 KB, patch)
2020-06-26 14:49 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (1.93 KB, patch)
2020-06-27 08:22 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2020-06-25 18:14:22 PDT
Improve assertions in StringParsingBuffer
Comment 1 Sam Weinig 2020-06-25 18:15:56 PDT Comment hidden (obsolete)
Comment 2 Sam Weinig 2020-06-25 18:51:28 PDT
Created attachment 402841 [details]
Comment 3 Darin Adler 2020-06-26 09:37:41 PDT
Comment on attachment 402841 [details]

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.
Comment 4 Sam Weinig 2020-06-26 14:49:25 PDT
Created attachment 402907 [details]
Comment 5 Darin Adler 2020-06-26 15:10:25 PDT
Comment on attachment 402907 [details]

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

<= ?
Comment 6 Sam Weinig 2020-06-27 08:22:26 PDT
Created attachment 402956 [details]
Comment 7 EWS 2020-06-27 09:07:46 PDT
Committed r263620: <https://trac.webkit.org/changeset/263620>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402956 [details].
Comment 8 Radar WebKit Bug Importer 2020-06-27 09:08:20 PDT
Comment 9 Radar WebKit Bug Importer 2020-06-27 09:08:24 PDT