959662 - NetdReceiver::DispatchNetdEvent::RunTask needs to root

Status: RESOLVED WONTFIX

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

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(Added in bug 735547.)

Comment 1

[Vincent Chang[:vchang][changyihsin]]

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 .....

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://blog.mozilla.org/javascript/2013/07/18/clawing-our-way-back-to-precision/ gives some background.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Vincent Chang[:vchang][changyihsin]]

Hi Ms2ger and Boris, thanks for your information, I'll post a patch to fix this soon.

Comment 5

[Al Billings [:abillings - ex-MoCo]]

How far back does this go in affected versions?

Comment 6

[Andrew McCreight (out of office until 8/21) [:mccr8]]

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.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(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.

Comment 8

[Andrew McCreight (out of office until 8/21) [:mccr8]]

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.

Comment 9

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.0

Hi Ms2ger, could you help to review the patch ? 

1. Rooted JSObject/JS::Value/JS::String for Netd.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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...

Comment 12

[Vincent Chang[:vchang][changyihsin]]

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.

Comment 13

[Vincent Chang[:vchang][changyihsin]]

Since Bug 864931 is landed, I would like to close this bug.