Crash [@ generator_op] iteration over gen.__proto__

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
12 years ago
7 years ago

People

(Reporter: Jesse Ruderman, Assigned: Igor Bukanov)

Tracking

(Blocks: 1 bug, {crash, testcase, verified1.8.1})

Trunk
PowerPC
Mac OS X
crash, testcase, verified1.8.1
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(1 attachment)

Fix
1.34 KB, patch
brendan
: review+
Mike Schroepfer
: approval1.8.1+
Details | Diff | Splinter Review
(Reporter)

Description

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

Null deref crash [@ generator_op].
(Assignee)

Updated

12 years ago
Assignee: general → igor.bukanov
(Assignee)

Comment 1

12 years ago
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");
(Assignee)

Comment 2

12 years ago
Created attachment 238710 [details] [diff] [review]
Fix

Fix that confirms to the test case from comment 1.

Again I have no intentions to commit until having a sleep.
Attachment #238710 - Flags: review?(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
Attachment #238710 - Flags: review?(brendan) → review+
(Assignee)

Comment 4

12 years ago
(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
> 

Status: NEW → ASSIGNED
(Assignee)

Comment 5

12 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Flags: blocking1.8.1.1?
Resolution: --- → FIXED
(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 on attachment 238710 [details] [diff] [review]
Fix

Safe null defense, good for stopping an annoyance DOS.

/be
Attachment #238710 - Flags: approval1.8.1?

Comment 8

12 years ago
Comment on attachment 238710 [details] [diff] [review]
Fix

a=schrep - approved to land on 1.8.1 branch before rc1.
Attachment #238710 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 9

12 years ago
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
Keywords: fixed1.8.1

Comment 10

12 years ago
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
Flags: in-testsuite+

Comment 11

12 years ago
verified fixed 1.8 1.9 20060919 windows/mac*/linux
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1 → verified1.8.1
Already in 1.8.1, clearing 1.8.1.1 request
Flags: blocking1.8.1.1?
(Reporter)

Updated

11 years ago
No longer blocks: 349611
(Reporter)

Updated

11 years ago
Blocks: 349611
Crash Signature: [@ generator_op]
You need to log in before you can comment on or make changes to this bug.