Closed Bug 1542184 Opened 8 months ago Closed 7 months ago

Creating Rooted objects on the heap is suspect in rust bindings

Categories

(Core :: JavaScript Engine, task, P1)

task

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: jonco, Assigned: jonco)

Details

Attachments

(1 file)

There's a couple of places in the rust bindings where Rooted<>s are created on the heap with new, e.g. CreateRootedObjectVector and CreateAutoIdVector*:

https://searchfox.org/mozilla-central/source/js/rust/src/jsglue.cpp#556
https://searchfox.org/mozilla-central/source/js/rust/src/jsglue.cpp#541

Rooted<> must be destroyed in LIFO order and I don't think there is any guarantee that this happens.

If not then these should probably be replaced with the equivalent PersistentRooted<>.

I don't think these are currently used in an unsafe manner but it looks like it's possible.

Flags: needinfo?(nfitzgerald)

Yeah, this is definitely a foot gun!

Flags: needinfo?(nfitzgerald)

Well, this seems like a big safety hole. Jon, do you have time to fix it?

Flags: needinfo?(jcoppeard)
Priority: -- → P1

Sure, I can look at this.

Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)

This replaces the use of heap-alloced Rooted with PersistentRooted which is safe wrt destruction order.

I had to add PersistentRooted and StackGCVector to OPAQUE_TYPES to make this work... I'm not really sure what this does.

Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4b8f9814a1ea
Use PersistentRooted for rooting vectors of GC things from rust code r=fitzgen?
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.