Closed Bug 959662 Opened 7 years ago Closed 7 years ago

NetdReceiver::DispatchNetdEvent::RunTask needs to root


(Core :: DOM: Core & HTML, defect)

Not set



Tracking Status
firefox26 --- wontfix
firefox27 --- ?
firefox28 --- ?
firefox29 --- affected


(Reporter: Ms2ger, Assigned: vchang)




(1 file)

(Added in bug 735547.)
Hi Ms2ger, may I know in more detail about your concern here ? netd is a native process with root privilege that we used in FFOS to configure network related stuffs such as wifi/usb hotpsot, network traffic threshold alarm .....
Vincent, the basic issue is that the code in question is not actually rooting its JS objects, so once we turn on exact scanning for GC those objects can be collected while the code is working with them.  The objects need to be rooted.
Hi Ms2ger and Boris, thanks for your information, I'll post a patch to fix this soon.
How far back does this go in affected versions?
Flags: needinfo?(Ms2ger)
It sounds like this is only a problem when exact rooting is turned on for B2G?  In that case, this doesn't really need to be tracked as a security bug.
(In reply to Al Billings [:abillings] from comment #5)
> How far back does this go in affected versions?

Since bug 735547 landed. The bug doesn't have a target milestone set, so I don't know when that was. It might not be a problem yet, though; the conservative stack scanner may well be sufficient until we turn on GGC.
Flags: needinfo?(Ms2ger)
Given that exact rooting isn't on, and is quite a ways from being on for b2g, I'm going to set this to sec-other.  It can probably be opened, but I'll just leave it here for now, to see how the patch looks.
Keywords: sec-criticalsec-other
Attached patch Patch v1.0Splinter Review
Hi Ms2ger, could you help to review the patch ? 

1. Rooted JSObject/JS::Value/JS::String for Netd.
Attachment #8362421 - Flags: review?(Ms2ger)
Comment on attachment 8362421 [details] [diff] [review]
Patch v1.0

Review of attachment 8362421 [details] [diff] [review]:

Use JS::Rooted<JS::Value> / JS::Rooted<JSObject*> rather than the typedefs.
Attachment #8362421 - Flags: review?(Ms2ger) → review?(bzbarsky)
Group: core-security
Keywords: sec-other
Comment on attachment 8362421 [details] [diff] [review]
Patch v1.0

Lots of cleanup opportunities here, fwiw:

>+  JS::RootedValue v(cx, JS_ARGV(cx, vp)[0]);

It'd really be nicer to use CallArgs here; then you wouldn't need the separate root.

>   if (JSVAL_IS_STRING(v)) {

v.isString() ?

>+    JS::RootedString str(cx, JSVAL_TO_STRING(v));

v.toString() ?

>   } else if (!JSVAL_IS_PRIMITIVE(v)) {

  } else if (v.isObject()) {

>+    JS::RootedObject obj(cx, JSVAL_TO_OBJECT(v));

Use v.toObject() here?

>+  JS::RootedObject array(aCx, JS_NewUint8Array(aCx, mMessage->mSize));

For what it's worth, you could use mozilla::dom::Uint8Array::Create and it would handle the memcpy bits for you...

>   JS::Value argv[] = { OBJECT_TO_JSVAL(array) };

This needs to be rooted (and this is the part that gets an r-).  I recommend JS::AutoValueVector...
Attachment #8362421 - Flags: review?(bzbarsky) → review-
Bug 864931 has rewrite the netd stuff in SystemWorkerManager.cpp. As I know, that bug is about to land because it helps to reduce the memory usage. After we land bug 864931, we don't need to fix this bug any more.
Since Bug 864931 is landed, I would like to close this bug.
Closed: 7 years ago
Resolution: --- → WONTFIX
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.