Make dictionaries and unions containing GC values safer #17056

jdm · 2017-05-26T18:51:48Z

Problems:

the Heap::new constructor is memory-unsafe with any value other than Undefined/Null
this means that moving dictionaries containing Heap values (ie. any/object) is memory-unsafe
unions containing GC pointers are similarly unsafe

Solutions:

dictionaries containing GC pointers are now wrapped in RootedTraceableBox (even inside other dictionaries)
instead of using Heap::new, dictionaries containing GC pointers are now initialized to a default value (by deriving Default) and mutated one field at a time
dictionaries containing GC pointers are marked #[must_root]
FromJSVal for dictionaries containing GC pointers now returns RootedTraceableBox
unions wrap their variants that require rooting in RootedTraceableBox

Rather than attempting to derive Default for all dictionaries, we only do so for the dictionaries that require it. Because some dictionaries that require it inherit from dictionaries that do not, we need to write manual implementations for those parent dictionaries. This is a better option than having to figure out a default value for types like Root<T>, which would be required for deriving Default for all dictionaries.

I would still like to come up with an automated test for this, but I figured I would get eyes on this first.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #16952
There are tests for these changes

This change is

highfive · 2017-05-26T18:51:53Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/testbinding.rs, components/script/dom/bindings/num.rs, components/script/dom/event.rs, components/script/dom/extendableevent.rs and 6 more
@KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/testbinding.rs, components/script/dom/bindings/num.rs, components/script/dom/event.rs, components/script/dom/extendableevent.rs and 6 more

highfive · 2017-05-26T18:51:54Z

Warning

These commits modify unsafe code. Please review it carefully!

jdm · 2017-05-26T18:52:11Z

r? @nox
After this PR there is only one use of Heap::new left in the whole codebase.

jdm · 2017-05-26T18:58:23Z

cc @MortimerGoro

MortimerGoro · 2017-05-29T14:54:05Z

I have tested the PR and it works great, no more GC crashes with dictionary initializers!

emilio · 2017-05-29T16:56:16Z

components/script/dom/bindings/codegen/CodegenRust.py

@@ -4002,12 +3999,18 @@ def __init__(self, enum):
    }
 }

+impl Default for super::%s {
+  fn default() -> super::%s {


nit: use 4 space indents. Also, perhaps it's worth to use named arguments?

emilio · 2017-05-29T16:56:16Z

components/script/dom/bindings/codegen/CodegenRust.py

+        selfName = self.makeClassName(d)
+        if self.membersNeedTracing():
+            actualType = "RootedTraceableBox<%s>" % selfName
+            preInitial = "let mut dictionary = RootedTraceableBox::new(%s::default());\n" % selfName


do you need the interpolation? I'd expected RootedTraceableBox::default() to just work.

jdm · 2017-05-29T21:11:49Z

I have had no luck creating a test for this yet.

bors-servo · 2017-07-21T23:55:55Z

☔ The latest upstream changes (presumably #17822) made this pull request unmergeable. Please resolve the merge conflicts.

MortimerGoro · 2017-08-08T15:39:50Z

@jdm can we merge this? I have to cherry pick the commits each time I want to test/prepare A-Frame demos in a new branch.

jdm · 2017-08-08T15:42:08Z

My goal was to write an automated test that demonstrated the problem, but it has been more challenging than I anticipated. I'll try to merge this sometime this week.

bors-servo · 2017-08-23T23:25:53Z

☔ The latest upstream changes (presumably #18179) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2017-08-24T01:42:55Z

@emilio I have addressed your first comment; the one about the RootedTraceableBox default caused compiler errors because it messed up some type inference.

bors-servo · 2017-09-21T19:56:53Z

☔ The latest upstream changes (presumably #18059) made this pull request unmergeable. Please resolve the merge conflicts.


        Don't generate union conversion functions for object variants.


        Be more conservative about safety of dictionary and union values.

Mark dictionaries containing GC values as must_root, and wrap them in RootedTraceableBox in automatically-generated APIs. To accommodate union variants that are now flagged as unsafe, add RootedTraceableBox to union variants that need to be rooted, rather than wrapping the entire union value.


        Store rootable dictionary members of dictionaries in RootedTraceableBox.


        Derive the Default trait for dictionaries containing GC values.


        Initialize rooted dictionaries to a stable value before setting fields.


        Remove almost all uses of Heap::new.


        Use more named string interpolation.

jdm · 2017-09-25T20:12:35Z

@bors-servo: r=emilio

bors-servo · 2017-09-25T20:12:35Z

📌 Commit 44822c3 has been approved by emilio

bors-servo · 2017-09-25T21:07:49Z

⌛ Testing commit 44822c3 with merge 532dee3...


        Auto merge of #17056 - jdm:heapdict, r=emilio

Make dictionaries and unions containing GC values safer Problems: * the Heap::new constructor is memory-unsafe with any value other than Undefined/Null * this means that moving dictionaries containing Heap values (ie. any/object) is memory-unsafe * unions containing GC pointers are similarly unsafe Solutions: - dictionaries containing GC pointers are now wrapped in RootedTraceableBox (even inside other dictionaries) - instead of using Heap::new, dictionaries containing GC pointers are now initialized to a default value (by deriving Default) and mutated one field at a time - dictionaries containing GC pointers are marked #[must_root] - FromJSVal for dictionaries containing GC pointers now returns RootedTraceableBox<Dictionary> - unions wrap their variants that require rooting in RootedTraceableBox Rather than attempting to derive Default for all dictionaries, we only do so for the dictionaries that require it. Because some dictionaries that require it inherit from dictionaries that do not, we need to write manual implementations for those parent dictionaries. This is a better option than having to figure out a default value for types like `Root<T>`, which would be required for deriving Default for all dictionaries. I would still like to come up with an automated test for this, but I figured I would get eyes on this first. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #16952 - [ ] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17056)

bors-servo · 2017-09-25T22:13:42Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: emilio
Pushing 532dee3 to master...

highfive assigned mbrubeck May 26, 2017

highfive added the S-awaiting-review label May 26, 2017

highfive assigned nox and unassigned mbrubeck May 26, 2017

jdm force-pushed the jdm:heapdict branch from bfb58c4 to 9ea964c May 26, 2017

emilio reviewed May 29, 2017

View changes

jdm mentioned this pull request Jun 15, 2017

Heap constructor is unsafe servo/rust-mozjs#343

Closed

highfive added the S-needs-rebase label Jul 21, 2017

highfive added the S-needs-rebase label Aug 23, 2017

jdm force-pushed the jdm:heapdict branch from 9ea964c to adff462 Aug 24, 2017

jdm removed the S-needs-rebase label Aug 24, 2017

jdm force-pushed the jdm:heapdict branch from adff462 to 3ba2dd8 Aug 24, 2017

highfive added the S-needs-rebase label Sep 21, 2017

jdm added 5 commits May 26, 2017

Don't generate union conversion functions for object variants.

e481e89

Store rootable dictionary members of dictionaries in RootedTraceableBox.

b169689

Derive the Default trait for dictionaries containing GC values.

16166d6

Initialize rooted dictionaries to a stable value before setting fields.

f5eb844

jdm added 2 commits May 26, 2017

Remove almost all uses of Heap::new.

77b3e91

Use more named string interpolation.

Loading status checks…

44822c3

jdm force-pushed the jdm:heapdict branch from 3ba2dd8 to 44822c3 Sep 25, 2017

highfive assigned emilio and unassigned nox Sep 25, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Sep 25, 2017

bors-servo merged commit 44822c3 into servo:master Sep 25, 2017
2 checks passed

2 checks passed

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Sep 25, 2017

servo / servo

Make dictionaries and unions containing GC values safer #17056

Make dictionaries and unions containing GC values safer #17056

jdm commented May 26, 2017 •

edited by larsbergstrom

highfive commented May 26, 2017

highfive commented May 26, 2017

jdm commented May 26, 2017

jdm commented May 26, 2017

MortimerGoro commented May 29, 2017

This comment has been minimized.

This comment has been minimized.

jdm commented May 29, 2017

bors-servo commented Jul 21, 2017

MortimerGoro commented Aug 8, 2017

jdm commented Aug 8, 2017

bors-servo commented Aug 23, 2017

jdm commented Aug 24, 2017

bors-servo commented Sep 21, 2017

jdm commented Sep 25, 2017

bors-servo commented Sep 25, 2017

bors-servo commented Sep 25, 2017

bors-servo commented Sep 25, 2017

servo / servo

Join GitHub today

Make dictionaries and unions containing GC values safer #17056

Make dictionaries and unions containing GC values safer #17056

Conversation

jdm commented May 26, 2017 • edited by larsbergstrom

highfive commented May 26, 2017

highfive commented May 26, 2017

jdm commented May 26, 2017

jdm commented May 26, 2017

MortimerGoro commented May 29, 2017

This comment has been minimized.

emilio May 29, 2017

This comment has been minimized.

emilio May 29, 2017

jdm commented May 29, 2017

bors-servo commented Jul 21, 2017

MortimerGoro commented Aug 8, 2017

jdm commented Aug 8, 2017

bors-servo commented Aug 23, 2017

jdm commented Aug 24, 2017

bors-servo commented Sep 21, 2017

jdm commented Sep 25, 2017

bors-servo commented Sep 25, 2017

bors-servo commented Sep 25, 2017

bors-servo commented Sep 25, 2017

jdm commented May 26, 2017 •

edited by larsbergstrom