710479 - ASan reports invalid read in PopOffPrec

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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.

Comment 1

[David Mandelin [:dmandelin]]

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.

Comment 2

[Christian Holler (:decoder)]

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.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: 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.

Comment 4

[David Mandelin [:dmandelin]]

Comment on attachment 622192 [details] [diff] [review]
v1

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

Die, gotox, die!

Comment 5

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bdd2ce20e52

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3bdd2ce20e52