1358754 - stylo: fix possibilities of overflow parsing nth-child

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Hiroyuki Ikezoe (:hiro)]

We should do the same thing as the fix for bug 1206105 in matches_generic_nth_child [1].

[1] https://hg.mozilla.org/mozilla-central/file/7d85b081bfab/servo/components/selectors
/matching.rs#l483

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6dbe6abfe6dcc916caa24523f570ae23fdfedecd

This try is based on a patch for bug 1321754 since the patch triggers this overflow somehow.  I have no idea why it triggers the overflow.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1358754 - Fix overflow in ::nth-child().

Review commit: https://reviewboard.mozilla.org/r/132614/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132614/

Comment 2

[Hiroyuki Ikezoe (:hiro)]

For comparison, this is a Ting-Yu's try with the patch; 1206105-1.html crashed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=109bf8fbc8ff0123ff40fecb4d6e23486c269f29&selectedJob=92877859

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 8860615 [details]
Bug 1358754 - Fix overflow in ::nth-child().

Nice, thanks for fixing this!

This seems reasonable, but I want Simon to make sure that we're matching the spec on the edge cases. Redirecting review.

Comment 4

[Simon Sapin (:SimonSapin)]

Comment on attachment 8860615 [details]
Bug 1358754 - Fix overflow in ::nth-child().

https://reviewboard.mozilla.org/r/132614/#review136174

This code does the right thing, but since we’re touching it I’d like to tweak it some more :)

::: servo/components/selectors/matching.rs:483
(Diff revision 1)
>      }
>  
>      if a == 0 {
>          b == index
>      } else {
> -        (index - b) / a >= 0 &&
> +        match index.checked_sub(b) {

Please change `let mut index = 1;` above to `let mut index: i32 = 1;` to make sure that `index` is signed and `checked_sub` doesn’t return None as soon as the result would be negative.

We do want to support a negative result here, but I care less about what happens when overflowing i32.

::: servo/components/selectors/matching.rs:486
(Diff revision 1)
>          b == index
>      } else {
> -        (index - b) / a >= 0 &&
> -        (index - b) % a == 0
> +        match index.checked_sub(b) {
> +            None => false,
> +            Some(r) => {
> +                r.checked_div(a).map_or(false, |r| r >= 0) &&

Since we’ve already checked `a == 0` above, `checked_div` and `checked_rem` should never return None. Still, it’s be nice to not have redundant checks. Perhaps replacing the whole if/else block with something like this?

```
// Is there a non-negative integer n such that An+B=index?
match index.checked_sub(b) {
    None => false,
    Some(an) => match an.checked_div(a) {
        Some(n) => n >= 0 && a * n == an,
        None /* a == 0 */ => an == 0,
    }
}
```

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8860615 [details]
Bug 1358754 - Fix overflow in ::nth-child().

https://reviewboard.mozilla.org/r/132614/#review136772

::: servo/components/selectors/matching.rs:486
(Diff revision 1)
>          b == index
>      } else {
> -        (index - b) / a >= 0 &&
> -        (index - b) % a == 0
> +        match index.checked_sub(b) {
> +            None => false,
> +            Some(r) => {
> +                r.checked_div(a).map_or(false, |r| r >= 0) &&

This code is very impressive! Thank you!

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Simon Sapin (:SimonSapin) from comment #4)
> Comment on attachment 8860615 [details]
> Bug 1358754 - Fix overflow in ::nth-child().
> 
> https://reviewboard.mozilla.org/r/132614/#review136174
> 
> This code does the right thing, but since we’re touching it I’d like to
> tweak it some more :)
> 
> ::: servo/components/selectors/matching.rs:483
> (Diff revision 1)
> >      }
> >  
> >      if a == 0 {
> >          b == index
> >      } else {
> > -        (index - b) / a >= 0 &&
> > +        match index.checked_sub(b) {
> 
> Please change `let mut index = 1;` above to `let mut index: i32 = 1;` to
> make sure that `index` is signed and `checked_sub` doesn’t return None as
> soon as the result would be negative.
> 
> We do want to support a negative result here, but I care less about what
> happens when overflowing i32.
> 
> ::: servo/components/selectors/matching.rs:486
> (Diff revision 1)
> >          b == index
> >      } else {
> > -        (index - b) / a >= 0 &&
> > -        (index - b) % a == 0
> > +        match index.checked_sub(b) {
> > +            None => false,
> > +            Some(r) => {
> > +                r.checked_div(a).map_or(false, |r| r >= 0) &&
> 
> Since we’ve already checked `a == 0` above, `checked_div` and `checked_rem`
> should never return None. Still, it’s be nice to not have redundant checks.
> Perhaps replacing the whole if/else block with something like this?
> 
> ```
> // Is there a non-negative integer n such that An+B=index?
> match index.checked_sub(b) {
>     None => false,
>     Some(an) => match an.checked_div(a) {
>         Some(n) => n >= 0 && a * n == an,
>         None /* a == 0 */ => an == 0,
>     }
> }

I meant this code, of course!

Comment 7

[Hiroyuki Ikezoe (:hiro)]

https://github.com/servo/servo/pull/16615

Comment 8

[Hiroyuki Ikezoe (:hiro)]

https://hg.mozilla.org/integration/autoland/rev/026696e3640b