[Security Review][Fuzzing] Ion-compile try-catch

RESOLVED FIXED

Status

mozilla.org
Security Assurance: Review Request
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jandem, Assigned: decoder)

Tracking

Details

(Whiteboard: [Fx])

(Reporter)

Description

4 years ago
Bug 866888 added Ion-compilation of try-catch. The patch is on inbound now (revision 383ccc9eb488) but it's disabled by default. To enable it, you have to pass 

    --ion-compile-try-catch

to the shell. Can I please get some fuzzing with this shell flag before I turn it on by default?
Flags: needinfo?(gary)
Flags: needinfo?(choller)
(Assignee)

Comment 1

4 years ago
How the hell did this even pass our regression tests?

$ debug64/js --ion-compile-try-catch --ion-eager -e 'try {} catch(e) {}'
Assertion failure: script()->analysis()->hasTryFinally(), at js/src/ion/IonBuilder.cpp:3232
Segmentation fault


I got more failures too.
Flags: needinfo?(choller)
(Assignee)

Comment 2

4 years ago
(In reply to Christian Holler (:decoder) from comment #1)
>
> I got more failures too.

Cannot process the other failures because they all get reduced to this one (it's too simple).
Flags: needinfo?(jdemooij)
(Reporter)

Comment 3

4 years ago
(In reply to Christian Holler (:decoder) from comment #1)
> How the hell did this even pass our regression tests?

I'm really sorry. I made some last-minute changes to address review comments and forgot to run tests with --ion-compile-try-catch.. Pushed a fix for that:

https://hg.mozilla.org/integration/mozilla-inbound/rev/73912c9ba403
Flags: needinfo?(jdemooij)
(Reporter)

Comment 4

4 years ago
I also did a Linux Try run with Ion try-catch compilation enabled in the browser and shell, looks green so far:

https://tbpl.mozilla.org/?tree=Try&rev=f23da4d71b30
(Reporter)

Comment 5

4 years ago
Fix is on m-c now :)
Flags: needinfo?(choller)
(Assignee)

Comment 6

4 years ago
The following tests crashes with --ion-eager --ion-compile-try-catch:

function x() {
  try {
    do {
      var { q , gen } = t;   
    } while(false);
  } catch (e) {}
} x();

====

o = undefined;
for(;;) {
  try {
    throw 1;
  } catch(e){
    break;
  }
}
for (var i in o) {}

====

The following test only crashes an optimized build for me (--disable-debug --enable-optimize):

var o = {
  valueOf: function() {}
};
function test(t) {
    try {
        for (x[t++] in o) {}
    } catch (err) {}
}
test(Infinity);
(Assignee)

Updated

4 years ago
Flags: needinfo?(choller)
(Reporter)

Updated

4 years ago
Depends on: 904079
(Reporter)

Updated

4 years ago
Depends on: 904133
I have a patch that tests this but will wait for decoder's bugs above to be fixed first before adding to the harness.
Flags: needinfo?(gary)
Assignee: nobody → choller
(Assignee)

Comment 8

4 years ago
Two more tests:


function Integer(value) {
  try {
    x();
  } catch ( e ) {  }
}
new Integer( NaN );
new Integer( 0 );


====

do {
  try{
    throw "ex1";
  }  catch(i) {
    break;
  }
} while(testProperty<2);
gczeal(6);
do {} while(0);



Could be that these are fixed already, but better test twice than missing a bug :)
(Reporter)

Comment 9

4 years ago
Yup, the first one is bug 904133, the second one is the same as the second testcase in comment 6. Both should be fixed in a few days.

(In reply to Christian Holler (:decoder) from comment #8)
> Could be that these are fixed already, but better test twice than missing a
> bug :)

Definitely :)
Blocks: 866888
Whiteboard: [Fx]
(Assignee)

Comment 10

4 years ago
This configuration/flag is now being tested continuously on fuzzer-linux7.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.