Closed
Bug 898494
Opened 8 years ago
Closed 8 years ago
Exactly root BinaryData
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: terrence, Assigned: nsm)
References
Details
Attachments
(1 file, 1 obsolete file)
11.28 KB,
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
The landing of BinaryData introduced a bunch of new GC hazards for exact rooting: http://people.mozilla.org/~sfink/analysis/ http://people.mozilla.org/~sfink/analysis/shell/rootingHazards.txt
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Assignee: general → nsm.nikhil
Assignee | ||
Comment 2•8 years ago
|
||
Comment on attachment 781918 [details] [diff] [review] Fix lack of rooting in Binary Data. I don't understand these two reports. |obj| is rooted, but what is this field:0? Function 'JSObject* js::ArrayType::create(JSContext*, class JS::Handle<JSObject*>, class JS::Handle<JSObject*>, uint32)' has unrooted '__temp_41' of type 'JSObject*' live across GC call 'BinaryData.cpp:uint64 GetMemSize(JSContext*, class JS::Handle<JSObject*>)' at js/src/builtin/BinaryData.cpp:642 js/src/builtin/BinaryData.cpp:642: Assign(89,90, __temp_44 := elementType*) js/src/builtin/BinaryData.cpp:642: Call(90,91, __temp_43 := GetMemSize(cx*,__temp_44*)) js/src/builtin/BinaryData.cpp:642: Call(91,92, __temp_42 := Int32Value((__temp_43* * length*))) js/src/builtin/BinaryData.cpp:642: Call(92,93, __temp_41*.field:0.setFixedSlot(0,__temp_42)) Function 'JSObject* js::ArrayType::create(JSContext*, class JS::Handle<JSObject*>, class JS::Handle<JSObject*>, uint32)' has unrooted '__temp_45' of type 'JSObject*' live across GC call 'BinaryData.cpp:uint64 GetAlign(JSContext*, class JS::Handle<JSObject*>)' at js/src/builtin/BinaryData.cpp:644 js/src/builtin/BinaryData.cpp:644: Assign(94,95, __temp_48 := elementType*) js/src/builtin/BinaryData.cpp:644: Call(95,96, __temp_47 := GetAlign(cx*,__temp_48*)) js/src/builtin/BinaryData.cpp:644: Call(96,97, __temp_46 := Int32Value(__temp_47*)) js/src/builtin/BinaryData.cpp:644: Call(97,98, __temp_45*.field:0.setFixedSlot(1,__temp_46))
Attachment #781918 -
Attachment description: Fix lack of rooting in Binary Data. r=? → Fix lack of rooting in Binary Data.
Attachment #781918 -
Flags: review?(nmatsakis)
Attachment #781918 -
Flags: feedback?(terrence)
Updated•8 years ago
|
Reporter | ||
Comment 3•8 years ago
|
||
(In reply to Nikhil Marathe [:nsm] from comment #2) > Comment on attachment 781918 [details] [diff] [review] > Fix lack of rooting in Binary Data. > > I don't understand these two reports. |obj| is rooted, but what is this > field:0? > > Function 'JSObject* js::ArrayType::create(JSContext*, class > JS::Handle<JSObject*>, class JS::Handle<JSObject*>, uint32)' has unrooted > '__temp_41' of type 'JSObject*' live across GC call 'BinaryData.cpp:uint64 > GetMemSize(JSContext*, class JS::Handle<JSObject*>)' at > js/src/builtin/BinaryData.cpp:642 > js/src/builtin/BinaryData.cpp:642: Assign(89,90, __temp_44 := > elementType*) > js/src/builtin/BinaryData.cpp:642: Call(90,91, __temp_43 := > GetMemSize(cx*,__temp_44*)) > js/src/builtin/BinaryData.cpp:642: Call(91,92, __temp_42 := > Int32Value((__temp_43* * length*))) > js/src/builtin/BinaryData.cpp:642: Call(92,93, > __temp_41*.field:0.setFixedSlot(0,__temp_42)) The code in question here is: obj->setFixedSlot(SLOT_MEMSIZE, Int32Value(::GetMemSize(cx, elementType) * length)); C++ ensures that all arguments that are calls get fully resolved before they are subsequently passed to a call, however, it does not enforce any ordering between those calls and other operations within the same sequence point. Thus, a perfectly valid ordering of the operations on this line is: (1) RootedObject::operator-> => Unpacks raw JSObject* to stack. (2) GetMemSize => GC's and corrupts stack copy of obj. (3) ObjectImpl::setFixedSlot => |this| is now the corrupt stack copy. You need to establish a sequence point between your GetMemSize and where you insert the thing into the heap. In general you should always assign the result of any GC function to a stack temporary. For example, an even more subtle variant of this problem is where you have operator-> and operator= on a HeapPtr: |obj->shape = MaybeGC(...); // kaboom!| > Function 'JSObject* js::ArrayType::create(JSContext*, class > JS::Handle<JSObject*>, class JS::Handle<JSObject*>, uint32)' has unrooted > '__temp_45' of type 'JSObject*' live across GC call 'BinaryData.cpp:uint64 > GetAlign(JSContext*, class JS::Handle<JSObject*>)' at > js/src/builtin/BinaryData.cpp:644 > js/src/builtin/BinaryData.cpp:644: Assign(94,95, __temp_48 := > elementType*) > js/src/builtin/BinaryData.cpp:644: Call(95,96, __temp_47 := > GetAlign(cx*,__temp_48*)) > js/src/builtin/BinaryData.cpp:644: Call(96,97, __temp_46 := > Int32Value(__temp_47*)) > js/src/builtin/BinaryData.cpp:644: Call(97,98, > __temp_45*.field:0.setFixedSlot(1,__temp_46)) And this appears to be the same on the next line.
Reporter | ||
Comment 4•8 years ago
|
||
Comment on attachment 781918 [details] [diff] [review] Fix lack of rooting in Binary Data. Review of attachment 781918 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/BinaryData.cpp @@ -822,5 @@ > if (valueStr) > valueChars = JS_EncodeString(cx, valueStr); > JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_INCOMPATIBLE_PROTO, "ArrayType", "repeat", valueChars); > - if (valueStr) > - JS_free(cx, valueChars); I think you still need to free these characters; at least, nothing in this patch is changing whether you would need to or not.
Attachment #781918 -
Flags: feedback?(terrence) → feedback+
Assignee | ||
Comment 5•8 years ago
|
||
Fixes pointed out by :terrence.
Attachment #781918 -
Attachment is obsolete: true
Attachment #781918 -
Flags: review?(nmatsakis)
Attachment #782000 -
Flags: review?(nmatsakis)
Updated•8 years ago
|
Attachment #782000 -
Flags: review?(nmatsakis) → review+
Assignee | ||
Comment 6•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf208ba34b9c
Comment 7•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bf208ba34b9c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in
before you can comment on or make changes to this bug.
Description
•