Closed Bug 899976 Opened 6 years ago Closed 6 years ago

GC: Fix unsafe references related to ToInt* functions

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: jonco, Assigned: jonco)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Attached patch to-int-32 (obsolete) — Splinter Review
There are a bunch of these unsafe reference warnings caused by the fact that these functions take references to a Value.  This bug is to convert them to take handles instead.
Attachment #783712 - Flags: review?(luke)
Attached patch to-int-browserSplinter Review
Attachment #783715 - Flags: review?(bobbyholley+bmo)
Comment on attachment 783712 [details] [diff] [review]
to-int-32

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

::: js/src/vm/TypedArrayObject.cpp
@@ +204,5 @@
>  
>      JSObject *bufobj = create(cx, uint32_t(nbytes));
>      if (!bufobj)
>          return false;
> +    args.setCallee(ObjectValue(*bufobj));

I think you want args.rval().set

@@ +1834,3 @@
>          if (!obj)
>              return false;
>          vp->setObject(*obj);

args.rval().set
Attachment #783712 - Flags: review?(luke) → review+
Ah, it looks like sfink has also just fixed this with bug 899760.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 899760
Attachment #783715 - Flags: review?(bobbyholley+bmo)
Comment on attachment 783712 [details] [diff] [review]
to-int-32

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

::: js/src/ion/AsmJS.cpp
@@ +5794,2 @@
>  
>      return true;

Wait, why does this return int32_t?

::: js/src/jsapi.h
@@ +1630,1 @@
>          js::MaybeCheckStackRoots(cx);

Do we still need those?

::: js/src/vm/TypedArrayObject.cpp
@@ +204,5 @@
>  
>      JSObject *bufobj = create(cx, uint32_t(nbytes));
>      if (!bufobj)
>          return false;
> +    args.setCallee(ObjectValue(*bufobj));

args.rval().setObject(*bufobj), even

@@ +1868,5 @@
>          int32_t byteOffset = 0;
>          int32_t length = -1;
>  
> +        if (args.length() > 1) {
> +            if (!ToInt32(cx, args.get(1), &byteOffset))

There should be no need to use get() here; you just checked args.length()!

@@ +1877,5 @@
>                  return NULL;
>              }
>  
> +            if (args.length() > 2) {
> +                if (!ToInt32(cx, args.get(2), &length))

Ditto
Ok, we're going to mark bug 899760 the duplicate and merge the patch there into this one.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Duplicate of this bug: 899760
(In reply to :Ms2ger from comment #4)
> ::: js/src/jsapi.h
> @@ +1630,1 @@
> >          js::MaybeCheckStackRoots(cx);
> 
> Do we still need those?

Yes, they make SM(r) on TBPL more likely to find bugs.
(In reply to :Ms2ger from comment #4)

Thanks for the comments.

> Wait, why does this return int32_t?

Rather than bool?  ValueToInt32() and ValueToNumber() are called from JIT code - I guess it may be something to do with calling convention.
Attachment #783715 - Flags: review?(bobbyholley+bmo)
Attached patch to-int-32Splinter Review
I merged in the changes from the patch in bug 899760 - sfink do you want to check this is ok?
Attachment #783712 - Attachment is obsolete: true
Attachment #784364 - Flags: review?(sphink)
Attachment #783715 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 784364 [details] [diff] [review]
to-int-32

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

::: js/src/jsapi.cpp
@@ +6755,5 @@
>  }
>  
>  #ifdef DEBUG
>  JS_PUBLIC_API(void)
> +JS::AssertArgumentsAreSane(JSContext *cx, const HandleValue value)

Why the const? Actually, I'm not sure why this compiles. The decl from jsapi.h is:

  AssertArgumentsAreSane(JSContext *cx, JS::Handle<JS::Value> v);

::: js/src/jsapi.h
@@ +1599,5 @@
>  
>  namespace js {
>  /* DO NOT CALL THIS.  Use JS::ToInt16. */
>  extern JS_PUBLIC_API(bool)
> +ToUint16Slow(JSContext *cx, const JS::Handle<JS::Value> v, uint16_t *out);

What does const Handle mean?

::: js/src/vm/TypedArrayObject.cpp
@@ +189,5 @@
>  ArrayBufferObject::class_constructor(JSContext *cx, unsigned argc, Value *vp)
>  {
>      int32_t nbytes = 0;
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    if (argc > 0 && !ToInt32(cx, args.get(0), &nbytes))

Can't this just be args[0]?

@@ +1844,3 @@
>          /* () or (number) */
>          uint32_t len = 0;
> +        if (args.length() == 0 || ValueIsLength(args.get(0), &len))

args[0]

@@ +1847,4 @@
>              return fromLength(cx, len);
>  
>          /* (not an object) */
> +        if (!args.get(0).isObject()) {

args[0]
Attachment #784364 - Flags: review?(sphink) → review+
https://hg.mozilla.org/mozilla-central/rev/f09bcd847699
https://hg.mozilla.org/mozilla-central/rev/09837c7d38e4
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.