The default bug view has changed. See this FAQ.

ASan reports invalid read in PopOffPrec

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: decoder, Assigned: jorendorff)

Tracking

(Blocks: 1 bug, {testcase})

Trunk
mozilla15
x86_64
Linux
testcase
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [asan])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
Address sanitizer has picked up a memory error for the following test (mozilla-central revision f16d056c532b (no options required)):

eval('y = {}; (y.a = [2 for each (p in [])])();');


Trace:

==12032== ERROR: AddressSanitizer global-buffer-overflow on address 0x0000024aa58b at pc 0x104792b bp 0x7fffbb7d1230 sp 0x7fffbb7d1228
READ of size 1 at 0x0000024aa58b thread T0
    #0 0x104792b in PopOffPrec js/src/jsopcode.cpp:1250
    #1 0x1046c26 in PopOff js/src/jsopcode.cpp:1271
    #2 0x100b292 in Decompile js/src/jsopcode.cpp:3306
    #3 0xff35c4 in DecompileCode js/src/jsopcode.cpp:4901
    #4 0xfed441 in DecompileExpression js/src/jsopcode.cpp:5304
    #5 0xfe855d in js_DecompileValueGenerator js/src/jsopcode.cpp:5198
    #6 0x85a968 in _ZN2jsL23DecompileValueGeneratorEP9JSContextiRKN2JS5ValueEP8JSString ../jsopcode.h:482
    #7 0x85b9a9 in _Z24js_ReportValueErrorFlagsP9JSContextjjiRKN2JS5ValueEP8JSStringPKcS8_ js/src/jscntxt.cpp:1219
    #8 0xa09cf2 in _Z22js_ReportIsNotFunctionP9JSContextPKN2JS5ValueEj js/src/jsfun.cpp:2403
    #9 0xd32034 in _ZN2js12InvokeKernelEP9JSContextNS_8CallArgsENS_14MaybeConstructE js/src/jsinterp.cpp:616
    #10 0xcc2878 in _ZN2js9InterpretEP9JSContextPNS_10StackFrameENS_10InterpModeE js/src/jsinterp.cpp:3499
    #11 0xc7fa5c in _ZN2js9RunScriptEP9JSContextP8JSScriptPNS_10StackFrameE js/src/jsinterp.cpp:580
    #12 0xd39077 in _ZN2js13ExecuteKernelEP9JSContextP8JSScriptR8JSObjectRKN2JS5ValueENS_11ExecuteTypeEPNS_10StackFrameEPS7_ js/src/jsinterp.cpp:775
    #13 0xe3f5e0 in EvalKernel js/src/jsobj.cpp:1259
    [...]
0x0000024aa58b is located 21 bytes to the left of global variable '.str' (0x24aa5a0) of size 2
  '.str' is ascii string '|'
0x0000024aa58b is located 19 bytes to the right of global variable 'js_CodeSpec' (0x24a9e60) of size 1816
==12032== ABORTING


Valgrind does not show any error (this is most likely a stack issue). If you need a JS build with ASan to debug this, let me know :) Flagging S-s until triaged due to possible memory safety problem.
I'm pretty sure this is not s-s: it's reading off the end of a static array by 18 bytes, so it shouldn't be able to read any user data.

The bad read is here:

    topcs = &js_CodeSpec[ss->opcodes[top]];
    if (topcs->prec != 0 && topcs->prec < prec) {

ss->opcodes[top] is 229, aka JSOP_FORLOCAL. It's not in jsopcode.tbl, so it is not in js_CodeSpec.
Group: core-security
Whiteboard: js-triage-needed
(Reporter)

Updated

5 years ago
Whiteboard: [asan]
(Reporter)

Comment 2

5 years ago
I added an assertion to perform a bisect:


diff --git a/js/src/jsopcode.cpp b/js/src/jsopcode.cpp
--- a/js/src/jsopcode.cpp
+++ b/js/src/jsopcode.cpp
@@ -1495,2 +1495,4 @@ PopOffPrec(SprintStack *ss, uint8_t prec
     off = GetOff(ss, top);
+
+    JS_ASSERT(ss->opcodes[top] < (sizeof(js_CodeSpec)/sizeof(js_CodeSpec[0])) );
     topcs = &js_CodeSpec[ss->opcodes[top]];


Bisect shows:

The first bad revision is:
changeset:   73034:938c1a177114
user:        Jason Orendorff
date:        Tue Jul 19 11:00:43 2011 -0500
summary:     Bug 648175 - Remove JSOP_FOR*. Second second landing, to coin a phrase. r=dvander.
(Assignee)

Updated

5 years ago
Assignee: general → jorendorff
(Assignee)

Comment 3

5 years ago
Created attachment 622192 [details] [diff] [review]
v1

I looked at this a little bit to see if there was a better way, but I think the right answer really is just to hack this particular line of code to cope with JSOP_FORLOCAL.

I agree with dmandelin this probably can't affect users, but it's worth patching anyway.
Attachment #622192 - Flags: review?(dmandelin)
Comment on attachment 622192 [details] [diff] [review]
v1

Review of attachment 622192 [details] [diff] [review]:
-----------------------------------------------------------------

Die, gotox, die!
Attachment #622192 - Flags: review?(dmandelin) → review+
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bdd2ce20e52
https://hg.mozilla.org/mozilla-central/rev/3bdd2ce20e52
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.