27924 - [UMR] GC reads from uninitialed memory from js_AllocStack

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[John Bandhauer]

js_Interpret calls js_AllocStack where the code has the comment:

  Allocate operand and pc stack slots for the script's worst-case depth.

Those slots are not all used. Apparently we don't bother to initialize them to a 
safe value.

In GC we do..

 for (vp = (jsval *)begin; vp < (jsval *)end; vp++) {
    v = *vp;
    if (JSVAL_IS_GCTHING(v))
      GC_MARK(rt, JSVAL_TO_GCTHING(v), "stack", NULL);
 }

.. and end up reading from uninitialized slots.

So, do we consider this harmless? Or is it worth making sure all the alloc'd 
slots are initialized?

Comment 1

[Phil Schwartau]

Reassigning to Patrick as per meeting -

Comment 2

[Patrick C. Beard]

Uninitialized slots could cause a GC crash. Nominating for nsbeta3.

Comment 3

[Brendan Eich [:brendan]]

Actually, the GC tries not to go beyond the currently-homed (in fp) sp.  But 
there's an exception for the thread on which the GC is currently called, dammit. 
Can someone show the actual purify backtrace so we can be sure the existing sp 
limiting code works, at least for other contexts?

/be

Comment 4

[John Bandhauer]

This is the trace I get from running xpcshell on xpctest_echo.js

[W] UMR: Uninitialized memory read in js_GC {16 occurrences}
        Reading 4 bytes from 0x0328ad4c (4 bytes at 0x0328ad4c uninitialized)
        Address 0x0328ad4c is 92 bytes into a 8211 byte block at 0x0328acf0
        Address 0x0328ad4c points to a malloc'd block in heap 0x02870000
        Thread ID: 0x110
        Error location
            js_GC          [jsgc.c:1004]
            js_ForceGC     [jsgc.c:814]
            ???            [ip=0x0347a370]
            GC             [xpcshell.cpp:306]
            js_Interpret   [jsinterp.c:2517]
            js_Execute     [jsinterp.c:887]
            JS_ExecuteScript [jsapi.c:2666]
            Process        [xpcshell.cpp:471]
            ProcessArgs    [xpcshell.cpp:630]
            main           [xpcshell.cpp:767]
        Allocation location
            malloc         [msvcrt.dll]
            JS_ArenaAllocate [jsarena.c:126]
            js_AllocStack  [jsinterp.c:338]
            js_Interpret   [jsinterp.c:1168]
            js_Execute     [jsinterp.c:887]
            JS_ExecuteScript [jsapi.c:2666]
            Process        [xpcshell.cpp:471]
            ProcessArgs    [xpcshell.cpp:630]
            main           [xpcshell.cpp:767]
            mainCRTStartup [crtexe.obj]

Comment 5

[Brendan Eich [:brendan]]

Patrick, do you mind if I take this back?  Other than the fact that it's all my 
fault, I think I made the underlying bug worse with a recent change.

/be

Comment 6

[leger]

Would like to see more reproducibility, higher profile.  Moving to P3.  Adding 
[PDTP3]..but if brendan can fix...awesome.

Comment 7

[Brendan Eich [:brendan]]

The fix is included in the patch for bug 49816.

/be

Comment 8

[Brendan Eich [:brendan]]

The old GC would bounds-check bad thing pointers by failing to find their flags
in a parallel arena pool, which I believe meant that you'd never crash on a wild
pointer read or write.  That doesn't reduce the severity of this bug by much,
and the fix is over in bug 49816, attached as a patch with commentary.

/be

Comment 9

[Brendan Eich [:brendan]]

Fix is in.

/be