Exactly root BinaryData

RESOLVED FIXED in mozilla25

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: terrence, Assigned: nsm)

Tracking

(Blocks: 1 bug)

Trunk
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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
Created attachment 781918 [details] [diff] [review]
Fix lack of rooting in Binary Data.
Assignee: general → nsm.nikhil
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)
Blocks: 898606
No longer blocks: 753203
(Reporter)

Comment 3

5 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

5 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+
Created attachment 782000 [details] [diff] [review]
Fix lack of rooting in Binary Data

Fixes pointed out by :terrence.
Attachment #781918 - Attachment is obsolete: true
Attachment #781918 - Flags: review?(nmatsakis)
Attachment #782000 - Flags: review?(nmatsakis)
Attachment #782000 - Flags: review?(nmatsakis) → review+
https://hg.mozilla.org/mozilla-central/rev/bf208ba34b9c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.