Fix unsafe Heap constructor usage in DOM objects #16500

MortimerGoro · 2017-04-17T17:50:22Z

Heap::new() constructor is unsafe. Heap should be set after reflect_dom_object call in order to prevent potential GC crashes.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-04-17T17:50:27Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/vreyeparameters.rs, components/script/dom/vrframedata.rs, components/script/dom/extendablemessageevent.rs, components/script/dom/vrstageparameters.rs, components/script/dom/messageevent.rs and 1 more
@KiChjang: components/script/dom/vreyeparameters.rs, components/script/dom/vrframedata.rs, components/script/dom/extendablemessageevent.rs, components/script/dom/vrstageparameters.rs, components/script/dom/messageevent.rs and 1 more

highfive · 2017-04-17T17:50:28Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify script code, but no tests are modified. Please consider adding a test!

asajeffrey · 2017-04-17T17:59:45Z

cc @jdm

asajeffrey

This is making me think we need to design the API to deal with servo/rust-mozjs#343 properly rather than implement lots of point fixes!

asajeffrey · 2017-04-17T18:50:32Z

components/script/dom/extendablemessageevent.rs

            origin: origin,
            lastEventId: lastEventId,
        };
-        let ev = reflect_dom_object(ev, global, ExtendableMessageEventBinding::Wrap);
+        let root = reflect_dom_object(ev, global, ExtendableMessageEventBinding::Wrap);


Not sure the renaming is buying us anything here.

asajeffrey · 2017-04-17T18:50:32Z

components/script/dom/gamepad.rs

@@ -75,27 +74,27 @@ impl Gamepad {
                       state: &WebVRGamepadState) -> Root<Gamepad> {
        let buttons = GamepadButtonList::new_from_vr(&global, &state.buttons);
        let pose = VRPose::new(&global, &state.pose);
+
+        let root = reflect_dom_object(box Gamepad::new_inherited(state.gamepad_id,


Nit: variable name?

asajeffrey · 2017-04-17T18:50:32Z

components/script/dom/gamepad.rs

-            let _ = Float64Array::create(cx,
-                                         CreateWith::Slice(&state.axes),
-                                         axes.handle_mut());
+           let _ = Float64Array::create(cx, CreateWith::Slice(&state.axes), array.handle_mut());


Is there a way to have Float64Array::create allocate directly into root.axes rather than into a dummy variable? @jdm?

There is Heap::handle_mut, which would avoid the need for a local stack root: http://doc.servo.org/js/jsapi/struct.Heap.html#method.handle_mut

asajeffrey · 2017-04-17T18:50:32Z

components/script/dom/messageevent.rs

            origin: origin,
            lastEventId: lastEventId,
        };
-        reflect_dom_object(ev, global, MessageEventBinding::Wrap)
+        let root = reflect_dom_object(ev, global, MessageEventBinding::Wrap);


Nit: variable name?

asajeffrey · 2017-04-17T18:50:32Z

components/script/dom/vreyeparameters.rs

+        let cx = global.get_cx();
+        rooted!(in (cx) let mut array = ptr::null_mut());
+        unsafe {
+            let _ = Float32Array::create(cx, CreateWith::Slice(&parameters.offset), array.handle_mut());


asajeffrey · 2017-04-17T18:50:32Z

components/script/dom/vrframedata.rs

+fn create_typed_array(cx: *mut JSContext, src: &[f32], dst: &Heap<*mut JSObject>) {
+    unsafe {
+        rooted!(in (cx) let mut array = ptr::null_mut());
+        let _ = Float32Array::create(cx, CreateWith::Slice(src), array.handle_mut());


asajeffrey · 2017-04-17T18:50:32Z

components/script/dom/vrstageparameters.rs

+        unsafe {
+           let _ = Float32Array::create(cx,
+                                        CreateWith::Slice(&parameters.sitting_to_standing_transform),
+                                        array.handle_mut());


jdm · 2017-04-17T22:24:04Z

All I've got so far is removing the constructor that accepts an argument. I have not been able to figure out any meaningful API change that can prevent problems like this.

asajeffrey · 2017-04-17T23:16:25Z

Is there anything clever we can do with destination-passing style to avoid moving heap objects? I vaguely remember talking to @pnkfelix about this, in a different context.

MortimerGoro · 2017-05-01T18:49:35Z

Hi, any news/ideas about fixing the Heap constructor API?

@jdm we were waiting for this response to continue the PR review: Is there a way to have Float64Array::create allocate directly into root.axes rather than into a dummy variable?

asajeffrey · 2017-05-02T15:12:40Z

Well, the remaining fixes aren't vital, we could land the PR without them. You can fix the build failure (the unit tests need updating) and then r=me. (@jdm speak up if you are unhappy about this!)

jdm · 2017-05-02T15:59:10Z

The unit test problems are unrelated to this PR and are being fixed in #16655.


        Fix unsafe Heap constructor usage in DOM objects

MortimerGoro · 2017-05-03T23:12:18Z

@asajeffrey PR updated allocating directly into heap rather than into a dummy variable, using the heap.handle_mut() method that @jdm recommended

asajeffrey · 2017-05-04T01:48:19Z

OK, at this point @bors-servo r+

bors-servo · 2017-05-04T01:48:20Z

📌 Commit efb59b7 has been approved by asajeffrey

bors-servo · 2017-05-04T02:07:34Z

⌛ Testing commit efb59b7 with merge e8aa375...


        Auto merge of #16500 - MortimerGoro:unsafe_heap, r=asajeffrey

Fix unsafe Heap constructor usage in DOM objects  See servo/rust-mozjs#343 (comment) Heap::new() constructor is unsafe. Heap should be set after reflect_dom_object call in order to prevent potential GC crashes.  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16500)

bors-servo · 2017-05-04T02:49:29Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: asajeffrey
Pushing e8aa375 to master...

highfive assigned asajeffrey Apr 17, 2017

highfive added the S-awaiting-review label Apr 17, 2017

asajeffrey reviewed Apr 17, 2017

View changes

asajeffrey added S-awaiting-answer and removed S-awaiting-review labels Apr 17, 2017

MortimerGoro force-pushed the MortimerGoro:unsafe_heap branch from cf73647 to d6aa818 Apr 17, 2017

highfive added the S-awaiting-review label Apr 17, 2017

MortimerGoro force-pushed the MortimerGoro:unsafe_heap branch from d6aa818 to 2edef1a Apr 28, 2017

Fix unsafe Heap constructor usage in DOM objects

Loading status checks…

efb59b7

MortimerGoro force-pushed the MortimerGoro:unsafe_heap branch from 2edef1a to efb59b7 May 3, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-awaiting-answer labels May 4, 2017

bors-servo merged commit efb59b7 into servo:master May 4, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label May 4, 2017

servo / servo

Fix unsafe Heap constructor usage in DOM objects #16500

Fix unsafe Heap constructor usage in DOM objects #16500

MortimerGoro commented Apr 17, 2017 •

edited

highfive commented Apr 17, 2017

highfive commented Apr 17, 2017

asajeffrey commented Apr 17, 2017

asajeffrey left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Apr 17, 2017

asajeffrey commented Apr 17, 2017

MortimerGoro commented May 1, 2017

asajeffrey commented May 2, 2017

jdm commented May 2, 2017

MortimerGoro commented May 3, 2017

asajeffrey commented May 4, 2017

bors-servo commented May 4, 2017

bors-servo commented May 4, 2017

bors-servo commented May 4, 2017

servo / servo

Join GitHub today

Fix unsafe Heap constructor usage in DOM objects #16500

Fix unsafe Heap constructor usage in DOM objects #16500

Conversation

MortimerGoro commented Apr 17, 2017 • edited

highfive commented Apr 17, 2017

highfive commented Apr 17, 2017

asajeffrey commented Apr 17, 2017

asajeffrey left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Apr 17, 2017

asajeffrey commented Apr 17, 2017

MortimerGoro commented May 1, 2017

asajeffrey commented May 2, 2017

jdm commented May 2, 2017

MortimerGoro commented May 3, 2017

asajeffrey commented May 4, 2017

bors-servo commented May 4, 2017

bors-servo commented May 4, 2017

bors-servo commented May 4, 2017

MortimerGoro commented Apr 17, 2017 •

edited