Closed Bug 838955 Opened 9 years ago Closed 9 years ago

Fix rooting hazards in jsstr.cpp identified by static analysis

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

(Whiteboard: [js:t])

Attachments

(2 files, 2 obsolete files)

jsstr.cpp is in my sights.
[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.
Attachment #711151 - Flags: review?(sphink)
Attachment #711150 - Attachment is obsolete: true
Attachment #711150 - Flags: review?(sphink)
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.
Attachment #711173 - Flags: review?(sphink)
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.
Attachment #711151 - Flags: review?(sphink) → review+
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 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;
}

?
Attachment #711173 - Flags: review?(sphink)
(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.
Whiteboard: [js:t] → [js:t][leave open]
Is this what you wanted with the handleAt()?
Attachment #711661 - Flags: review?(sphink)
Attachment #711173 - Attachment is obsolete: true
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.
Attachment #711661 - Flags: review?(sphink) → review+
Whiteboard: [js:t][leave open] → [js:t]
https://hg.mozilla.org/mozilla-central/rev/d87f3b860eaa
https://hg.mozilla.org/mozilla-central/rev/2c6da9f02606
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.