The default bug view has changed. See this FAQ.

js_DecompileCode should not print if Decompile failed

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({fixed1.8.1.15})

Trunk
fixed1.8.1.15
Points:
---
Bug Flags:
blocking1.8.1.15 +
wanted1.8.1.x +
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:nse?])

Attachments

(2 attachments)

(Assignee)

Description

9 years ago
As the bug 431248 shows, a failed LOCAL_ASSERT does not save the decompiler and leads to crashes without DEBUG. The reason for this is that DecompileCode does not check that Decompile returns true before trying to print the last result:

    ok = Decompile(&ss, pc, len, JSOP_NOP) != NULL;
    if (code != oldcode) {
        JS_free(cx, jp->script->code);
        jp->script->code = oldcode;
        jp->script->main = oldmain;
    }
    jp->script = oldscript;

    /* If the given code didn't empty the stack, do it now. */
    if (ss.top) {
^^^^^^^^^^^^^^^^^^ - the missing ok check
        do {
            last = OFF2STR(&ss.sprinter, PopOff(&ss, JSOP_POP));
        } while (ss.top > pcdepth);
        js_printf(jp, "%s", last);
    }
 
Note that ok can be false here not only due to bugs in the code but also due to oom errors.
Flags: blocking1.9?
Flags: blocking1.8.1.15?
(Assignee)

Comment 1

9 years ago
Created attachment 318556 [details] [diff] [review]
v1

Adding the missed ok check.
Attachment #318556 - Flags: review?(brendan)
(Assignee)

Comment 2

9 years ago
To test the patch, one can use the test case from the bug 431248 without the fix there.
Status: NEW → ASSIGNED
Comment on attachment 318556 [details] [diff] [review]
v1

This is totally safe to ride along into 1.9. You can see ok set unconditionally above the added test, in the context.

/be
Attachment #318556 - Flags: review?(brendan)
Attachment #318556 - Flags: review+
Attachment #318556 - Flags: approval1.9?
Comment on attachment 318556 [details] [diff] [review]
v1

a=shaver: protection against other crashes in cases of LOCAL_ASSERT failure is righteous.
Attachment #318556 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 5

9 years ago
I checked in the patch from the comment 1 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1209562200&maxdate=1209562263&who=igor%25mir2.org
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Flags: blocking1.8.1.15? → blocking1.8.1.15+
Flags: wanted1.8.1.x+

Comment 6

9 years ago
see bug 431248 js1_7/extensions/regress-431248.js
Flags: in-testsuite+
Flags: in-litmus-

Comment 7

9 years ago
v 1.9.0
Status: RESOLVED → VERIFIED
(Assignee)

Updated

9 years ago
Flags: blocking1.9?
(Assignee)

Comment 8

9 years ago
Created attachment 323035 [details] [diff] [review]
v1 for 1.8 branch

The patch is a trivial backport of one-liner from the trunk.
Attachment #318556 - Attachment is obsolete: true
Attachment #323035 - Flags: review?(brendan)
(Assignee)

Updated

9 years ago
Attachment #318556 - Attachment is obsolete: false
Whiteboard: needs r=brendan
Attachment #323035 - Flags: review?(brendan)
Attachment #323035 - Flags: review+
Attachment #323035 - Flags: approval1.8.1.15?
Whiteboard: needs r=brendan
Comment on attachment 323035 [details] [diff] [review]
v1 for 1.8 branch

Approved for 1.8.1.15, a=dveditz for release-drivers
Attachment #323035 - Flags: approval1.8.1.15? → approval1.8.1.15+
Is there a testcase for this on the branch? this bug refers to bug 431248 which appears to be a trunk-only regression.
(Assignee)

Comment 11

9 years ago
(In reply to comment #10)
> Is there a testcase for this on the branch? 

I am not aware of a test case that shows the problem on the branch.

(Assignee)

Comment 12

9 years ago
I checked in the patch from the comment 8 to the MOZILLA_1_8_BRANCH:

Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.89.2.75; previous revision: 3.89.2.74
done
Keywords: fixed1.8.1.15
Whiteboard: [sg:nse?]
Group: security
You need to log in before you can comment on or make changes to this bug.