The default bug view has changed. See this FAQ.

Crash during GC after uneval (involves E4X, mysterious sharp variable)

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Jesse Ruderman, Assigned: Igor Bukanov)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
x86
Mac OS X
crash, regression, testcase, verified1.8.1.15
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.15 +
wanted1.8.1.x +
wanted1.8.0.x -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical] regression from 308429)

Attachments

(2 attachments)

(Reporter)

Description

10 years ago
js> www = <x><y/></x>; print(uneval(this) + "\n"); gc();
({www:#1=<x>
  <y/>
</x>})


Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_INVALID_ADDRESS (0x0001) at 0xdddddc00

Thread 0 Crashed:
0   js 	0x00053264 js_GetGCThingFlags + 26 (jsgc.c:512)
1   js 	0x00055663 JS_CallTracer + 495 (jsgc.c:2146)
2   js 	0x0008696b js_TraceObject + 811 (jsobj.c:4898)
3   js 	0x000548ec JS_TraceChildren + 140 (jsgc.c:1812)
4   js 	0x0005577d JS_CallTracer + 777 (jsgc.c:2172)
5   js 	0x0008696b js_TraceObject + 811 (jsobj.c:4898)
6   js 	0x000548ec JS_TraceChildren + 140 (jsgc.c:1812)
7   js 	0x0005577d JS_CallTracer + 777 (jsgc.c:2172)
8   js 	0x00055ccb js_TraceStackFrame + 355 (jsgc.c:2333)
9   js 	0x0005639a js_TraceContext + 124 (jsgc.c:2445)
10  js 	0x000568d5 js_TraceRuntime + 179 (jsgc.c:2519)
11  js 	0x00056b3b js_GC + 530 (jsgc.c:2747)
12  js 	0x000161a7 JS_GC + 96 (jsapi.c:2372)
13  js 	0x000037f6 GC + 45 (js.c:742)
14  js 	0x00059941 js_Invoke + 2945 (jsinterp.c:1332)
15  js 	0x0006a376 js_Interpret + 62065 (jsinterp.c:4025)
16  js 	0x0005a295 js_Execute + 715 (jsinterp.c:1591)
17  js 	0x0001b02f JS_ExecuteScript + 54 (jsapi.c:4692)
18  js 	0x00002692 Process + 912 (js.c:268)
19  js 	0x0000309b ProcessArgs + 2045 (js.c:519)
20  js 	0x00007fb0 main + 612 (js.c:3256)
21  js 	0x000021a6 _start + 216
22  js 	0x000020cd start + 41
(Reporter)

Comment 1

10 years ago
With less contamination:

js> o = {}; o.a = <x><y/></x>; print(uneval(o)); gc();
({a:#1=<x>
  <y/>
</x>})
Segmentation fault

(Assignee)

Updated

10 years ago
Assignee: general → igor
(Assignee)

Comment 2

10 years ago
The problem is caused by the fact that during uneval(obj) the runtime will call
JS_Enumerate of the xml object and that eventually will invoke
xml_lookupProperty with id == 0 corresponding to the first child of the xml.
That in turn calls js_AddNativeProperty and that locks the object and calls
js_AddScopeProperty.

In this particular case js_AddScopeProperty will allocate a new slot via
calling js_AllocSlot. That slot is explicitly left uninitialized under the
assumption that somebody will eventually set it before unlocking the scope. But
this is never done in js_AddNativeProperty leaving the junk value (or explicit
0xddddddd8 marker with a debug build) in the slot and eventually crashing
during tracing the object graph during GC.

So I guess js_AddNativeProperty has to be patched to initialize the slot to
something, but I am not sure under which conditions.
(Assignee)

Comment 3

10 years ago
Here is almost trivial case to expose the same problem with uninitialized slot after js_AddNativeProperty:

js> var x = <x><y/></x>; 0 in x; gc();
Segmentation fault

(Reporter)

Comment 4

10 years ago
Is the "mysterious" (seemingly unnecessary) use of a sharp variable here related to that problem, or is it a separate bug?
(Reporter)

Comment 5

10 years ago
Igor, do you know which patch from the last few days led to this regression?
(Assignee)

Comment 6

10 years ago
(In reply to comment #5)
> Igor, do you know which patch from the last few days led to this regression?

I think this is an old problem, will test it later today.
(Assignee)

Comment 7

10 years ago
(In reply to comment #4)
> Is the "mysterious" (seemingly unnecessary) use of a sharp variable here
> related to that problem, or is it a separate bug?

This is a separated and unrelated bug.
(Assignee)

Comment 8

10 years ago
This is a recent regression, see bug 308429. I was wrong to advise to set the slot to 0xddddddd8 there. If for some reason under way-too-much GC it was necessary to set the slot to void in AllocSlot, then there was bug somewhere else. At that point the free slot must be void as either ReallocSlot or js_Clear/js_FreeSlot sets the available slots to void.
Blocks: 308429
(Assignee)

Comment 9

10 years ago
Created attachment 265031 [details] [diff] [review]
fix v1
Attachment #265031 - Flags: review?(brendan)
Comment on attachment 265031 [details] [diff] [review]
fix v1

Whew, glad this was caught quickly.

/be
Attachment #265031 - Flags: review?(brendan) → review+
(Assignee)

Comment 11

10 years ago
I committed the patch from comment 9 to the trunk:

Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.337; previous revision: 3.336
done
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Reporter)

Comment 12

10 years ago
I filed bug 380946 on the unnecessary sharping.
(Reporter)

Updated

10 years ago
No longer blocks: 349611
(Reporter)

Updated

10 years ago
Blocks: 349611
Flags: wanted1.8.1.x-
Flags: wanted1.8.0.x-
Whiteboard: post 1.8-branch

Comment 13

10 years ago
Created attachment 266301 [details]
e4x/Regress/regress-380833.js

Updated

10 years ago
Flags: in-testsuite+
Whiteboard: post 1.8-branch → [sg:critical] post 1.8-branch
Group: security

Comment 14

10 years ago
verified fixed 1.9.0 linux/mac*/windows.

/cvsroot/mozilla/js/tests/e4x/Regress/regress-380833.js,v  <--  regress-380833.js
initial revision: 1.1
Status: RESOLVED → VERIFIED
Wanted on the 1.8 branch if we take bug 308429
Flags: wanted1.8.1.x-
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.15?
Whiteboard: [sg:critical] post 1.8-branch → [sg:critical] regression from 308429
Blocks: 426628
Flags: blocking1.8.1.15? → blocking1.8.1.15+

Updated

9 years ago
Keywords: fixed1.8.1.15

Comment 16

9 years ago
verified fixed 1.8.1.15 linux/mac/win
Keywords: fixed1.8.1.15 → verified1.8.1.15
You need to log in before you can comment on or make changes to this bug.