unexpected TypeError while calling a function

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: soubok, Unassigned)

Tracking

({regression})

Trunk
x86
Windows XP
regression
Points:
---
Bug Flags:
blocking1.9.2 +

Firefox Tracking Flags

(status1.9.2 beta4-fixed, status1.9.1 unaffected)

Details

(Whiteboard: [sg:moderate] (read from stack location) fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3
Build Identifier: trunk

The following script reports a TypeError at the 4st loop.
I cannot reproduce the issue in the js.exe shell.

var list = [];
function Add() {
   
    list.push(arguments);
}

function Run() {
   
    for each ( var item in list )
        item[0](); // x!x!x!debug.js:10: TypeError: item[0] is not a function
}

for ( var i = 0; i < 10; i++ ) {
    Add(function(s) { print('x!') });
}

Run();



Reproducible: Always
(Reporter)

Updated

9 years ago
Version: unspecified → Trunk
(Reporter)

Comment 1

9 years ago
If I remove JSOPTION_JIT or JSOPTION_COMPILE_N_GO, the error disappear.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

9 years ago
Dave, note the list.push(arguments) part. That seems to be involved.

Updated

9 years ago
Flags: blocking1.9.2?
Comment on attachment 407900 [details] [diff] [review]
fix

Brief description for posterity: the bug (introduced by me) was on a path that reads an argument through the arguments object that is stored on trace, but passed to the interpreter, so the value needs to be unboxed. And for that, you need the type. I arranged for the typemap to be accessible to this builtin. I did that, but I forgot to replace prototype code that used a fixed type of 1 (integer).
Attachment #407900 - Flags: review?(dmandelin) → review+
Group: core-security
Created attachment 407911 [details] [diff] [review]
better fix

Whoops, as discussed on IRC this wasn't a full fix. Reading from |apn| there is invalid anyway since the trace is no longer active. js_PutArguments has to clear the argsobj's private.
Attachment #407900 - Attachment is obsolete: true
Attachment #407911 - Flags: review?(dmandelin)
I marked this security sensitive because it lets you read random memory, albeit tagged as an integer. This can be verified by running the first patch + test case under valgrind where you get an invalid read error.
Attachment #407911 - Flags: review?(dmandelin) → review+

Updated

9 years ago
Whiteboard: fixed-in-tracemonkey → [sg:moderate] (read from stack location) fixed-in-tracemonkey

Comment 10

9 years ago
http://hg.mozilla.org/mozilla-central/rev/34be4f52a1df
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: blocking1.9.2? → blocking1.9.2+
Resolution: --- → FIXED
Duplicate of this bug: 527380
Duplicate of this bug: 529719
(In reply to comment #4)
> Brief description for posterity: the bug (introduced by me) [...]

Is there a specific bug number for the patch that caused the regression? If so please add it to this bug's "Blocks:" list. I'm guessing we have no plans to land whatever it was on older branches (marking 1.9.1 "unaffected" for now), but good to have just in case some future set of bugs needs it as a prerequisite.
status1.9.1: --- → unaffected
Keywords: regression
Group: core-security
You need to log in before you can comment on or make changes to this bug.