1340581 - release-assert against overflows in mozilla::Tokenizer

Status: RESOLVED FIXED

Description

[Honza Bambas (:mayhemer)]

There are some potentially dangerous use of how strings/dep-strings are constructed.  Should be armed with release grade assertions.

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

broom in front of my door-step :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9943fdadf45b8c383e0b6c77b2278d71d36fccc2

Comment 2

[Honza Bambas (:mayhemer)]

Better try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05437466e352c84e0dafe43685d4a9536037aa02

Comment 3

[Nathan Froyd [:froydnj] [away through 21 July]]

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.

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v1.1

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec3487e17b2f
Add some release-grade assertions to mozilla::Tokenizer to catch string overflows. r=froydnj

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ec3487e17b2f