838955 - Fix rooting hazards in jsstr.cpp identified by static analysis

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

jsstr.cpp is in my sights.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Fix rooting hazards in jsstr.cpp identified by static analysis.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Fix rooting hazards in jsstr.cpp identified by static analysis.

[Sorry, I forgot the patch description.]

> Function 'jsstr.cpp:uint8 FindReplaceLength(JSContext*, js::RegExpStatics*, ReplaceData*, uint64*)' has unrooted 'lambda' live across GC call 'uint8 js::ContextStack::pushInvokeArgs(JSContext*, uint32, js::InvokeArgsGuard*, uint32)' at js/src/jsstr.cpp:2066

Easy fix, done.

> Function 'jsstr.cpp:uint8 FindReplaceLength(JSContext*, js::RegExpStatics*, ReplaceData*, uint64*)' has unrooted 'staticsGuard' live across GC call 'uint8 js::ContextStack::pushInvokeArgs(JSContext*, uint32, js::InvokeArgsGuard*, uint32)' at js/src/jsstr.cpp:2066

I don't understand this one.

> Function 'jsstr.cpp:uint8 str_replace_flat_lambda(JSContext*, JS::CallArgs, ReplaceData*, FlatMatch*)' has unrooted 'matchStr' live across GC call 'uint8 js::ContextStack::pushInvokeArgs(JSContext*, uint32, js::InvokeArgsGuard*, uint32)' at js/src/jsstr.cpp:2519

Easy fix, done.

> Function 'int32 js::str_split(JSContext*, uint32, JS::Value*)' has unrooted 'sepstr' live across GC call 'uint8 js::RegExpToShared(JSContext*, JSObject*, js::RegExpGuard*)' at js/src/jsstr.cpp:2954

Easy fix, done.

> Function 'jsstr.cpp:int32 str_decodeURI_Component(JSContext*, uint32, JS::Value*)' has unrooted 'result' live across GC call 'jsstr.cpp:int32 Decode(JSContext*, JSString*, uint16*, JS::Value*)' at js/src/jsstr.cpp:4299

I think this is a false positive -- result isn't set before the call to Decode,
but Decode() itself sets it.  I could just root it anyway?  Alternatively,
Decode only GCs when it issues an error message, so I could just suppress GC
for that.

> Function 'jsstr.cpp:int32 str_encodeURI(JSContext*, uint32, JS::Value*)' has unrooted 'result' live across GC call 'jsstr.cpp:int32 Encode(JSContext*, JSString*, uint16*, uint16*, JS::Value*)' at js/src/jsstr.cpp:4315 

Ditto.

> Function 'jsstr.cpp:int32 str_encodeURI_Component(JSContext*, uint32, JS::Value*)' has unrooted 'result' live across GC call 'jsstr.cpp:int32 Encode(JSContext*, JSString*, uint16*, uint16*, JS::Value*)' at js/src/jsstr.cpp:4331

Ditto.

> Function 'JSString* js::ValueToSource(JSContext*, JS::Value*)' has unrooted 'rval' live across GC call 'int32 js::GetMethod(JSContext*, class JS::Handle<JSObject*>, JSHandleId, uint32, class JS::MutableHandle<JS::Value>)' at js/src/jsstr.cpp:3703

Ditto.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Fix rooting hazards in jsstr.cpp identified by static analysis.

This patch fixes this hazard:

> Function 'JSString* js::ToStringSlow(JSContext*, JS::Value*) [with js::AllowGC allowGC = (js::AllowGC)1u; JSContext = JSContext]' has unrooted 'v' live across GC call 'jsobjinlines.h:uint8 js::ToPrimitive(JSContext*, uint32, JS::Value*)' at js/src/jsstr.cpp:3651

I had to change ToPrimitive to take a MutableHandleValue argument.

Please check the following parts closely.

- The |args0| case in jsdate.cpp.

- |v2| in jsnum.cpp (esp. w.r.t. the SkipRoots at the top of ToNumberSlow)

- |tmp| in DirectProxyHandler::defaultValue()

- All the fromMarkedLocation() examples in JaegerMonkey.

Comment 4

[Steve Fink [:sfink] [:s:]]

Comment on attachment 711151 [details] [diff] [review]
(part 1) - Fix rooting hazards in jsstr.cpp identified by static analysis.

Review of attachment 711151 [details] [diff] [review]:
-----------------------------------------------------------------

Ugh, apparently Ms2ger got here first. 2 of the 3 changes in this patch just landed this morning.

Comment 5

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

Comment on attachment 711173 [details] [diff] [review]
(part 2) - Fix rooting hazards in jsstr.cpp identified by static analysis.

Review of attachment 711173 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsdate.cpp
@@ +3057,2 @@
>              return false;
> +        args[0] = args0;

args.handleAt(0), maybe

::: js/src/jsobjinlines.h
@@ +1471,1 @@
>      return true;

Could be "return JSObject::..."

@@ +1484,1 @@
>      return true;

Same here

Comment 6

[Steve Fink [:sfink] [:s:]]

Comment on attachment 711173 [details] [diff] [review]
(part 2) - Fix rooting hazards in jsstr.cpp identified by static analysis.

Review of attachment 711173 [details] [diff] [review]:
-----------------------------------------------------------------

I want to re-review whatever you do for the ToStringSlow case.

Thanks!

::: js/src/jsdate.cpp
@@ +3057,2 @@
>              return false;
> +        args[0] = args0;

What Ms2ger said (args.handleAt(0)), even though it gives me the creeps since I don't think it's currently safe to assume that vp points to rooted memory. (It usually does, but sometimes the caller is constructing a bogus chunk of Values. Maybe we should require them to root those?)

::: js/src/jsobjinlines.h
@@ +1465,5 @@
>          }
>      }
>  
>      RootedObject objRoot(cx, obj);
> +    if (!JSObject::defaultValue(cx, objRoot, JSTYPE_VOID, vp))

just return this now

@@ +1480,3 @@
>          return true;
> +    RootedObject obj(cx, &vp.toObject());
> +    if (!JSObject::defaultValue(cx, obj, preferredType, vp))

just return this now

::: js/src/jsstr.cpp
@@ +3644,5 @@
>  {
>      /* As with ToObjectSlow, callers must verify that |arg| isn't a string. */
>      JS_ASSERT(!arg.isString());
>  
> +    RootedValue v(cx, arg);

Sadly, I don't think we want this root when allowGC is false. It's only a tiny bit of perf loss, but it seems likely to trip up an analysis where we check that we're not rooting in allowGC=false situations.

It only needs to be rooted for ToPrimitive, right? So maybe this could be:

Value v = arg;
if (!arg.isPrimitive()) {
  ...
  RootedValue root(cx, arg)
  ToPrimitive(&root);
  v = root;
}

?

Comment 7

[Brian Hackett [Laid off!]]

(In reply to Steve Fink [:sfink] from comment #6)
> ::: js/src/jsdate.cpp
> @@ +3057,2 @@
> >              return false;
> > +        args[0] = args0;
> 
> What Ms2ger said (args.handleAt(0)), even though it gives me the creeps
> since I don't think it's currently safe to assume that vp points to rooted
> memory. (It usually does, but sometimes the caller is constructing a bogus
> chunk of Values. Maybe we should require them to root those?)

The caller should be required to root all values that are passed to Invoke or to Natives.  I don't known if all callers currently have this property though.

Comment 8

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d87f3b860eaa

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Fix rooting hazards in jsstr.cpp identified by static analysis.

Is this what you wanted with the handleAt()?

Comment 10

[Steve Fink [:sfink] [:s:]]

Comment on attachment 711661 [details] [diff] [review]
(part 2) - Fix rooting hazards in jsstr.cpp identified by static analysis.

Review of attachment 711661 [details] [diff] [review]:
-----------------------------------------------------------------

Yes! Just so. Thanks.

Comment 11

[Nicholas Nethercote [inactive]]

Part 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c6da9f02606

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d87f3b860eaa
https://hg.mozilla.org/mozilla-central/rev/2c6da9f02606