352885 - Crash [@ generator_op] iteration over gen.__proto__

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> for (j in ((function() { yield 3; })().__proto__)) print(j);

Null deref crash [@ generator_op].

Comment 1

[Igor Bukanov]

I will attach that treat gen.__proto__ as closed generator so the following test passes:

function test() 
{
  var proto = (function() { yield 3; })().__proto__;

  try {
    proto.next();
    throw "generatorProto.next() does not throw StopIteration";
  } catch (e) {
    if (!(e instanceof StopIteration))
      throw "generatorProto.next() throws unexpected exception: "+uneval(e);
  }

  try {
    proto.send();
    throw "generatorProto.send() does not throw StopIteration";
  } catch (e) {
    if (!(e instanceof StopIteration))
      throw "generatorProto.send() throws unexpected exception: "+uneval(e);
  }

  var obj = {};
  try {
    proto.throw(obj);
    throw "generatorProto.throw(obj) does not throw obj";
  } catch (e) {
    if (e !== obj)
      throw "generatorProto.throw() throws unexpected exception: "+uneval(e);
  }

  var obj = {};
  try {
    proto.close();
  } catch (e) {
      throw "generatorProto.throw() throws exception: "+uneval(e);
  }

}

test();
print("Success");

Comment 2

[Igor Bukanov]

Attachment: Fix

Fix that confirms to the test case from comment 1.

Again I have no intentions to commit until having a sleep.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 238710 [details] [diff] [review]
Fix

Alternative is to create a closed generator in js_InitIteratorClasses, but that's heavyweight.

This is a simple crash fix.  The crash is a null deref, so no security hazard on sane OSes and in sane apps (see recent work using SEH on Windows to exploit null derefs; some crazy Linuxes map nearly null pages too).

Be a shame to miss 1.8.1, but I will wait for Igor to sleep before nominating.

/be

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> (From update of attachment 238710 [details] [diff] [review] [edit])
> Alternative is to create a closed generator in js_InitIteratorClasses, but
> that's heavyweight.

The code in the rest 3 places that accesses generator's private checks if it is not null. So adding that not-null check adds the consistency. I think having 4 checks for null takes less space then code to create a closed generator object for gen.__proto__ which in addition would waste 124+malloc overhead bytes for JSGenerator per each global scope. 

> 
> This is a simple crash fix.  The crash is a null deref, so no security hazard
> on sane OSes and in sane apps (see recent work using SEH on Windows to exploit
> null derefs; some crazy Linuxes map nearly null pages too).
> 
> Be a shame to miss 1.8.1, but I will wait for Igor to sleep before nominating.
> 
> /be
> 

Comment 5

[Igor Bukanov]

I committed the patch from comment 2 to the trunk:

Checking in jsiter.c;
/cvsroot/mozilla/js/src/jsiter.c,v  <--  jsiter.c
new revision: 3.41; previous revision: 3.40
done

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 238710 [details] [diff] [review] [edit] [edit])
> > Alternative is to create a closed generator in js_InitIteratorClasses, but
> > that's heavyweight.
> 
> The code in the rest 3 places that accesses generator's private checks if it is
> not null. So adding that not-null check adds the consistency. I think having 4
> checks for null takes less space then code to create a closed generator object
> for gen.__proto__ which in addition would waste 124+malloc overhead bytes for
> JSGenerator per each global scope. 

Yeah, heavyweight ;-).  The patch is good.  I'll nominate it for 1.8.1.

/be

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 238710 [details] [diff] [review]
Fix

Safe null defense, good for stopping an annoyance DOS.

/be

Comment 8

[Mike Schroepfer]

Comment on attachment 238710 [details] [diff] [review]
Fix

a=schrep - approved to land on 1.8.1 branch before rc1.

Comment 9

[Igor Bukanov]

I committed the patch from comment 2 to the trunk:

Checking in jsiter.c;
/cvsroot/mozilla/js/src/jsiter.c,v  <--  jsiter.c
new revision: 3.17.2.17; previous revision: 3.17.2.16
done

Comment 10

[Bob Clary [:bc] (inactive)]

Checking in regress-352885-01.js;
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-352885-01.js,v  <--  regress-352885-01.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_7/geniter/regress-352885-02.js,v
done
Checking in regress-352885-02.js;
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-352885-02.js,v  <--  regress-352885-02.js
initial revision: 1.1
done

Comment 11

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 1.9 20060919 windows/mac*/linux

Comment 12

[Daniel Veditz [:dveditz]]

Already in 1.8.1, clearing 1.8.1.1 request