Closed
Bug 1307296
Opened 8 years ago
Closed 8 years ago
Check for rooting hazards arising from pointers into possibly-inlined typed object memory
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: sfink, Assigned: sfink)
References
Details
(Keywords: sec-audit, Whiteboard: [adv-main52-])
Attachments
(2 files)
16.42 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
29.43 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
Bug for catching things like bug 1296640 with the rooting hazard analysis. Elem* val = typedObj->typedMem(); ...GC... f(val[0]); <-- hazard! memory might have moved
Assignee | ||
Comment 1•8 years ago
|
||
I don't know if you'd prefer I used TypedObjectElemArray whenever possible. But it's probably always going to be a mixture -- sometimes we're memcpy'ing or something, so single-element access isn't good enough. I suppose I could add a get() or autoconvert. I'm kind of tempted to change the exposed typedMem() to require a nogc parameter, with a private accessor for internal use. But maybe that's overkill?
Attachment #8797307 -
Flags: review?(jcoppeard)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Comment 2•8 years ago
|
||
Comment on attachment 8797307 [details] [diff] [review] Avoid GC hazards when accessing typed object data Review of attachment 8797307 [details] [diff] [review]: ----------------------------------------------------------------- Nice. ::: js/src/builtin/SIMD.cpp @@ +188,5 @@ > template bool js::ToSimdConstant<Bool32x4>(JSContext* cx, HandleValue v, jit::SimdConstant* out); > > template<typename Elem> > static Elem > +TypedObjectMemory(HandleValue v, const JS::AutoAssertOnGC&) If this is not called anywhere outside of the TypedObjectElemArray constructor, can we inline it into that?
Attachment #8797307 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Jon Coppeard (:jonco) from comment #2) > Comment on attachment 8797307 [details] [diff] [review] > Avoid GC hazards when accessing typed object data > > Review of attachment 8797307 [details] [diff] [review]: > ----------------------------------------------------------------- > > Nice. > > ::: js/src/builtin/SIMD.cpp > @@ +188,5 @@ > > template bool js::ToSimdConstant<Bool32x4>(JSContext* cx, HandleValue v, jit::SimdConstant* out); > > > > template<typename Elem> > > static Elem > > +TypedObjectMemory(HandleValue v, const JS::AutoAssertOnGC&) > > If this is not called anywhere outside of the TypedObjectElemArray > constructor, can we inline it into that? It is still called in several places where TypedObjectElemArray is not applicable.
Assignee | ||
Comment 4•8 years ago
|
||
Pull out the big stick. This would have prevented at least one sec-crit that I know of (now fixed).
Attachment #8797829 -
Flags: review?(jcoppeard)
Comment 5•8 years ago
|
||
Comment on attachment 8797829 [details] [diff] [review] Stricter requirements for accessing typed object data Review of attachment 8797829 [details] [diff] [review]: ----------------------------------------------------------------- Great, this will be much safer. ::: js/src/builtin/SIMD.cpp @@ +1322,5 @@ > } > > TypedObjectElemArray<MaskTypeElem> mask(args[0]); > + TypedObjectElemArray<Elem> tv(args[1]); > + TypedObjectElemArray<Elem> fv(args[2]); Not sure what's up with this part. I can see the replacement already in the tree.
Attachment #8797829 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Jon Coppeard (:jonco) from comment #5) > Comment on attachment 8797829 [details] [diff] [review] > Stricter requirements for accessing typed object data > > Review of attachment 8797829 [details] [diff] [review]: > ----------------------------------------------------------------- > > Great, this will be much safer. > > ::: js/src/builtin/SIMD.cpp > @@ +1322,5 @@ > > } > > > > TypedObjectElemArray<MaskTypeElem> mask(args[0]); > > + TypedObjectElemArray<Elem> tv(args[1]); > > + TypedObjectElemArray<Elem> fv(args[2]); > > Not sure what's up with this part. I can see the replacement already in the > tree. Oh, whoops. That's a bug in the earlier patch, fixed in this one. It causes a test failure. I'll move this hunk before landing. I had one other similar copy/paste bug, I'll double-check that I put it in the right patch too.
Comment 7•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6afa90f3a06c https://hg.mozilla.org/mozilla-central/rev/0ac4f99472a3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Updated•8 years ago
|
Group: core-security → core-security-release
Updated•7 years ago
|
Whiteboard: [adv-main52-]
You need to log in
before you can comment on or make changes to this bug.
Description
•