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

Status: RESOLVED FIXED

(mozilla.org :: Security Assurance: Review Request, task)

Description

[Jan de Mooij [:jandem]]

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?

Comment 1

[Christian Holler (:decoder)]

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.

Comment 2

[Christian Holler (:decoder)]

(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).

Comment 3

[Jan de Mooij [:jandem]]

(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

Comment 4

[Jan de Mooij [:jandem]]

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

Comment 5

[Jan de Mooij [:jandem]]

Fix is on m-c now :)

Comment 6

[Christian Holler (:decoder)]

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);

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I have a patch that tests this but will wait for decoder's bugs above to be fixed first before adding to the harness.

Comment 8

[Christian Holler (:decoder)]

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 :)

Comment 9

[Jan de Mooij [:jandem]]

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 :)

Comment 10

[Christian Holler (:decoder)]

This configuration/flag is now being tested continuously on fuzzer-linux7.