1399941 - stylo: thread '<unnamed>' panicked at 'attempt to multiply with overflow', servo/components/style/gecko/media_queries.rs:732:17

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Bob Clary [:bc] (inactive)]

Attachment: Windows Debug Log

[Tracking Requested - why for this release]: stylo debug panic

1. https://chitranj.standardchess.com/play.html
2. thread '<unnamed>' panicked at 'attempt to multiply with overflow', /mozilla/builds/nightly/mozilla/servo/components/style/gecko/media_queries.rs:732:17

Nightly 57 Windows and Linux.

Comment 1

[Brad Werth [:bradwerth]]

The big numbers in the integer ratio media queries are causing a multiplication overflow in our optimized ratio comparison. I'll change it to attempt the multiplication with overflow detection on, and if overflow occurs, instead do a slower test that calculates the ratios and compares them directly.

Comment 2

[Bobby Holley (:bholley)]

Thanks Brad!

Comment 3

[Brad Werth [:bradwerth]]

Attachment: Bug 1399941 Part 1: Prevent aspect-ratio media queries from causing multiplication overflow by extending values to u64.

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

Comment 4

[Brad Werth [:bradwerth]]

Attachment: Bug 1399941 Part 2: Add more aspect-ratios to a test of media queries, testing for overflow.

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

Comment 5

[Brad Werth [:bradwerth]]

Comment on attachment 8908366 [details]
Bug 1399941 Part 2: Add more aspect-ratios to a test of media queries, testing for overflow.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179988/diff/1-2/

Comment 6

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #1)
> The big numbers in the integer ratio media queries are causing a
> multiplication overflow in our optimized ratio comparison. I'll change it to
> attempt the multiplication with overflow detection on, and if overflow
> occurs, instead do a slower test that calculates the ratios and compares
> them directly.

I worked up a solution that did this (checking for div by zero, etc.) and it was super-ugly Rust. Then I realized that the u32 values could be cast to u64s, making the existing multiplication safe. Attachment 8908365 [details] follows this approach. Also created some tests.

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8908365 [details]
Bug 1399941 Part 1: Prevent aspect-ratio media queries from causing multiplication overflow by extending values to u64.

https://reviewboard.mozilla.org/r/179986/#review185226

r=me with the nits addressed.

::: servo/components/style/gecko/media_queries.rs:734
(Diff revision 1)
>              (&BoolInteger(one), &BoolInteger(ref other)) => one.cmp(other),
>              (&Float(one), &Float(ref other)) => one.partial_cmp(other).unwrap(),
>              (&IntRatio(one_num, one_den), &IntRatio(other_num, other_den)) => {
> -                (one_num * other_den).partial_cmp(&(other_num * one_den)).unwrap()
> +                // Large u32s may overflow in multiplication, so we up them
> +                // to u64s. This is safe because u32 max ^ 2 < u64 max.
> +                debug_assert!((<u32>::max_value() as u64) ^ 2 < <u64>::max_value());

There's no need to waste cycles in debug builds to know this is true, please remove.

A comment saying something like:

```
// Extend to avoid overflow.
```

May be enough.

::: servo/components/style/gecko/media_queries.rs:735
(Diff revision 1)
>              (&Float(one), &Float(ref other)) => one.partial_cmp(other).unwrap(),
>              (&IntRatio(one_num, one_den), &IntRatio(other_num, other_den)) => {
> -                (one_num * other_den).partial_cmp(&(other_num * one_den)).unwrap()
> +                // Large u32s may overflow in multiplication, so we up them
> +                // to u64s. This is safe because u32 max ^ 2 < u64 max.
> +                debug_assert!((<u32>::max_value() as u64) ^ 2 < <u64>::max_value());
> +                (one_num as u64 * other_den as u64).partial_cmp(

While you're here, looks weird that this is using `partial_cmp(..).unwrap()`

This can definitely use:

(one_num as u64 * other_den as u64).cmp(&(other_num as u64 * one_den as u64))

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8908366 [details]
Bug 1399941 Part 2: Add more aspect-ratios to a test of media queries, testing for overflow.

https://reviewboard.mozilla.org/r/179988/#review185228

These are unnecessary. They're testing servo code, to begin with (because that's what test-unit tests with), and Servo and Gecko code for media-query parsing and evaluation is radically different.

Also, servo doesn't support aspect-ratio media queries, so you're effectively just testing the behavior of parsing an invalid MediaList.

Please add either a crashtest in Gecko, or a test to a style test like `test_media_queries.html`.

Comment 9

[Brad Werth [:bradwerth]]

Comment on attachment 8908365 [details]
Bug 1399941 Part 1: Prevent aspect-ratio media queries from causing multiplication overflow by extending values to u64.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179986/diff/1-2/

Comment 10

[Brad Werth [:bradwerth]]

Comment on attachment 8908366 [details]
Bug 1399941 Part 2: Add more aspect-ratios to a test of media queries, testing for overflow.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179988/diff/2-3/

Comment 11

[Brad Werth [:bradwerth]]

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

Comment 12

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8908366 [details]
Bug 1399941 Part 2: Add more aspect-ratios to a test of media queries, testing for overflow.

https://reviewboard.mozilla.org/r/179988/#review185626

Comment 13

[Brad Werth [:bradwerth]]

Comment on attachment 8908366 [details]
Bug 1399941 Part 2: Add more aspect-ratios to a test of media queries, testing for overflow.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179988/diff/3-4/

Comment 14

[Bobby Holley (:bholley)]

Brad, did this land?

Comment 15

[Brad Werth [:bradwerth]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #14)
> Brad, did this land?

Servo fix has landed; Gecko tests have not landed yet.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c89b96997d04
Part 2: Add more aspect-ratios to a test of media queries, testing for overflow. r=emilio

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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