Last Comment Bug 710479 - ASan reports invalid read in PopOffPrec
: ASan reports invalid read in PopOffPrec
Status: RESOLVED FIXED
[asan]
: testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
: -- critical (vote)
: mozilla15
Assigned To: Jason Orendorff [:jorendorff]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: langfuzz
  Show dependency treegraph
 
Reported: 2011-12-13 17:19 PST by Christian Holler (:decoder)
Modified: 2012-05-16 19:49 PDT (History)
12 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (1.99 KB, patch)
2012-05-08 16:18 PDT, Jason Orendorff [:jorendorff]
dmandelin: review+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2011-12-13 17:19:38 PST
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] 2011-12-15 18:13:59 PST
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) 2012-05-08 05:30:11 PDT
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] 2012-05-08 16:18:04 PDT
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.
Comment 4 David Mandelin [:dmandelin] 2012-05-08 16:25:07 PDT
Comment on attachment 622192 [details] [diff] [review]
v1

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

Die, gotox, die!
Comment 5 Jason Orendorff [:jorendorff] 2012-05-16 07:50:21 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bdd2ce20e52
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-05-16 19:49:04 PDT
https://hg.mozilla.org/mozilla-central/rev/3bdd2ce20e52

Note You need to log in before you can comment on or make changes to this bug.