release-assert against overflows in mozilla::Tokenizer

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

There are some potentially dangerous use of how strings/dep-strings are constructed.  Should be armed with release grade assertions.
Posted patch v1 (obsolete) — Splinter Review
broom in front of my door-step :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9943fdadf45b8c383e0b6c77b2278d71d36fccc2
Attachment #8838643 - Flags: review?(nfroyd)
Comment on attachment 8838643 [details] [diff] [review]
v1

Review of attachment 8838643 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below changes.  Thanks!

::: xpcom/ds/Tokenizer.cpp
@@ +568,5 @@
>  
> +  // This is not very likely to happen according to how we call this method
> +  // and since it's on a hot path, it's just a diagnostic assert, 
> +  // not a release assert.
> +  MOZ_DIAGNOSTIC_ASSERT(caret <= mEnd, "Overflow?");

For consistency's sake with the previous two asserts, it seems like it'd be better to test:

  mEnd >= caret

which is the same thing, but more closely linked to the pointer subtraction you're actually doing, and it's the same test you've been doing prior to this point.

@@ +629,5 @@
>  void
>  TokenizerBase::Token::AssignFragment(nsACString::const_char_iterator begin,
>                                       nsACString::const_char_iterator end)
>  {
> +  MOZ_RELEASE_ASSERT(begin <= end, "Overflow!");

Same comment here.
Attachment #8838643 - Flags: review?(nfroyd) → review+
Posted patch v1.1Splinter Review
Attachment #8838643 - Attachment is obsolete: true
Attachment #8839937 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec3487e17b2f
Add some release-grade assertions to mozilla::Tokenizer to catch string overflows. r=froydnj
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ec3487e17b2f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.