From 61d335141cb114df27ea04e8763503af490859e5 Mon Sep 17 00:00:00 2001 From: Jon Coppeard Date: Fri, 2 Aug 2013 13:15:38 +0100 Subject: [PATCH] Bug 899976 - GC: Fix unsafe references related to ToInt* functions - js engine changes r=sfink --- js/public/RootingAPI.h | 8 ++++-- js/src/ion/AsmJS.cpp | 6 ++--- js/src/jsapi.cpp | 2 +- js/src/jsapi.h | 49 ++++++++++++---------------------- js/src/jsexn.cpp | 31 +++++++++------------ js/src/jsnum.cpp | 10 +++---- js/src/vm/Interpreter.cpp | 8 +++--- js/src/vm/TypedArrayObject.cpp | 28 +++++++++---------- 8 files changed, 62 insertions(+), 80 deletions(-) diff --git a/js/public/RootingAPI.h b/js/public/RootingAPI.h index 57c6f0bba6b8..99295f123882 100644 --- a/js/public/RootingAPI.h +++ b/js/public/RootingAPI.h @@ -508,9 +508,13 @@ class MOZ_STACK_CLASS MutableHandle : public js::MutableHandleBase } T *address() const { return ptr; } - T get() const { return *ptr; } + const T& get() const { return *ptr; } - operator T() const { return get(); } + /* + * Return a reference so passing a MutableHandle to something that takes + * a |const T&| is not a GC hazard. + */ + operator const T&() const { return get(); } T operator->() const { return get(); } private: diff --git a/js/src/ion/AsmJS.cpp b/js/src/ion/AsmJS.cpp index 3d7b75ac6735..a30dbe32a969 100644 --- a/js/src/ion/AsmJS.cpp +++ b/js/src/ion/AsmJS.cpp @@ -5709,12 +5709,12 @@ GenerateFFIInterpreterExit(ModuleCompiler &m, const ModuleCompiler::ExitDescript } static int32_t -ValueToInt32(JSContext *cx, Value *val) +ValueToInt32(JSContext *cx, MutableHandleValue val) { int32_t i32; - if (!ToInt32(cx, val[0], &i32)) + if (!ToInt32(cx, val, &i32)) return false; - val[0] = Int32Value(i32); + val.set(Int32Value(i32)); return true; } diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp index 69ba0b372e65..ae6a73a5af7c 100644 --- a/js/src/jsapi.cpp +++ b/js/src/jsapi.cpp @@ -6774,7 +6774,7 @@ AutoGCRooter::AutoGCRooter(ContextFriendFields *cx, ptrdiff_t tag) #ifdef DEBUG JS_PUBLIC_API(void) -JS::AssertArgumentsAreSane(JSContext *cx, const JS::Value &value) +JS::AssertArgumentsAreSane(JSContext *cx, HandleValue value) { AssertHeapIsIdle(cx); CHECK_REQUEST(cx); diff --git a/js/src/jsapi.h b/js/src/jsapi.h index 4ab15bc0d4c0..1b0a0aa02307 100644 --- a/js/src/jsapi.h +++ b/js/src/jsapi.h @@ -75,9 +75,9 @@ class JS_PUBLIC_API(AutoCheckRequestDepth) * Also check that GC would be safe at this point. */ JS_PUBLIC_API(void) -AssertArgumentsAreSane(JSContext *cx, const Value &v); +AssertArgumentsAreSane(JSContext *cx, JS::Handle v); #else -inline void AssertArgumentsAreSane(JSContext *cx, const Value &v) { +inline void AssertArgumentsAreSane(JSContext *cx, JS::Handle v) { /* Do nothing */ } #endif /* DEBUG */ @@ -1600,35 +1600,32 @@ JS_ValueToUint64(JSContext *cx, jsval v, uint64_t *ip); namespace js { /* DO NOT CALL THIS. Use JS::ToInt16. */ extern JS_PUBLIC_API(bool) -ToUint16Slow(JSContext *cx, const JS::Value &v, uint16_t *out); +ToUint16Slow(JSContext *cx, JS::Handle v, uint16_t *out); /* DO NOT CALL THIS. Use JS::ToInt32. */ extern JS_PUBLIC_API(bool) -ToInt32Slow(JSContext *cx, const JS::Value &v, int32_t *out); +ToInt32Slow(JSContext *cx, JS::Handle v, int32_t *out); /* DO NOT CALL THIS. Use JS::ToUint32. */ extern JS_PUBLIC_API(bool) -ToUint32Slow(JSContext *cx, const JS::Value &v, uint32_t *out); +ToUint32Slow(JSContext *cx, JS::Handle v, uint32_t *out); /* DO NOT CALL THIS. Use JS::ToInt64. */ extern JS_PUBLIC_API(bool) -ToInt64Slow(JSContext *cx, const JS::Value &v, int64_t *out); +ToInt64Slow(JSContext *cx, JS::Handle v, int64_t *out); /* DO NOT CALL THIS. Use JS::ToUint64. */ extern JS_PUBLIC_API(bool) -ToUint64Slow(JSContext *cx, const JS::Value &v, uint64_t *out); +ToUint64Slow(JSContext *cx, JS::Handle v, uint64_t *out); } /* namespace js */ namespace JS { JS_ALWAYS_INLINE bool -ToUint16(JSContext *cx, const JS::Value &v, uint16_t *out) +ToUint16(JSContext *cx, JS::Handle v, uint16_t *out) { AssertArgumentsAreSane(cx, v); - { - js::SkipRoot skip(cx, &v); - js::MaybeCheckStackRoots(cx); - } + js::MaybeCheckStackRoots(cx); if (v.isInt32()) { *out = uint16_t(v.toInt32()); @@ -1638,13 +1635,10 @@ ToUint16(JSContext *cx, const JS::Value &v, uint16_t *out) } JS_ALWAYS_INLINE bool -ToInt32(JSContext *cx, const JS::Value &v, int32_t *out) +ToInt32(JSContext *cx, JS::Handle v, int32_t *out) { AssertArgumentsAreSane(cx, v); - { - js::SkipRoot root(cx, &v); - js::MaybeCheckStackRoots(cx); - } + js::MaybeCheckStackRoots(cx); if (v.isInt32()) { *out = v.toInt32(); @@ -1654,13 +1648,10 @@ ToInt32(JSContext *cx, const JS::Value &v, int32_t *out) } JS_ALWAYS_INLINE bool -ToUint32(JSContext *cx, const JS::Value &v, uint32_t *out) +ToUint32(JSContext *cx, JS::Handle v, uint32_t *out) { AssertArgumentsAreSane(cx, v); - { - js::SkipRoot root(cx, &v); - js::MaybeCheckStackRoots(cx); - } + js::MaybeCheckStackRoots(cx); if (v.isInt32()) { *out = uint32_t(v.toInt32()); @@ -1670,13 +1661,10 @@ ToUint32(JSContext *cx, const JS::Value &v, uint32_t *out) } JS_ALWAYS_INLINE bool -ToInt64(JSContext *cx, const JS::Value &v, int64_t *out) +ToInt64(JSContext *cx, JS::Handle v, int64_t *out) { AssertArgumentsAreSane(cx, v); - { - js::SkipRoot skip(cx, &v); - js::MaybeCheckStackRoots(cx); - } + js::MaybeCheckStackRoots(cx); if (v.isInt32()) { *out = int64_t(v.toInt32()); @@ -1687,13 +1675,10 @@ ToInt64(JSContext *cx, const JS::Value &v, int64_t *out) } JS_ALWAYS_INLINE bool -ToUint64(JSContext *cx, const JS::Value &v, uint64_t *out) +ToUint64(JSContext *cx, JS::Handle v, uint64_t *out) { AssertArgumentsAreSane(cx, v); - { - js::SkipRoot skip(cx, &v); - js::MaybeCheckStackRoots(cx); - } + js::MaybeCheckStackRoots(cx); if (v.isInt32()) { /* Account for sign extension of negatives into the longer 64bit space. */ diff --git a/js/src/jsexn.cpp b/js/src/jsexn.cpp index 66e9f8cb482d..fa42555716bf 100644 --- a/js/src/jsexn.cpp +++ b/js/src/jsexn.cpp @@ -1007,7 +1007,6 @@ IsDuckTypedErrorObject(JSContext *cx, HandleObject exnObject, const char **filen JSBool js_ReportUncaughtException(JSContext *cx) { - jsval roots[6]; JSErrorReport *reportp, report; if (!JS_IsExceptionPending(cx)) @@ -1017,8 +1016,8 @@ js_ReportUncaughtException(JSContext *cx) if (!JS_GetPendingException(cx, exn.address())) return false; - PodArrayZero(roots); - AutoArrayRooter tvr(cx, ArrayLength(roots), roots); + AutoValueVector roots(cx); + roots.resize(6); /* * Because ToString below could error and an exception object could become @@ -1048,18 +1047,12 @@ js_ReportUncaughtException(JSContext *cx) (exnObject->is() || IsDuckTypedErrorObject(cx, exnObject, &filename_str))) { RootedString name(cx); - if (JS_GetProperty(cx, exnObject, js_name_str, tvr.handleAt(2)) && - JSVAL_IS_STRING(roots[2])) - { - name = JSVAL_TO_STRING(roots[2]); - } + if (JS_GetProperty(cx, exnObject, js_name_str, roots.handleAt(2)) && roots[2].isString()) + name = roots[2].toString(); RootedString msg(cx); - if (JS_GetProperty(cx, exnObject, js_message_str, tvr.handleAt(3)) && - JSVAL_IS_STRING(roots[3])) - { - msg = JSVAL_TO_STRING(roots[3]); - } + if (JS_GetProperty(cx, exnObject, js_message_str, roots.handleAt(3)) && roots[3].isString()) + msg = roots[3].toString(); if (name && msg) { RootedString colon(cx, JS_NewStringCopyZ(cx, ": ")); @@ -1077,22 +1070,22 @@ js_ReportUncaughtException(JSContext *cx) str = msg; } - if (JS_GetProperty(cx, exnObject, filename_str, tvr.handleAt(4))) { - JSString *tmp = ToString(cx, HandleValue::fromMarkedLocation(&roots[4])); + if (JS_GetProperty(cx, exnObject, filename_str, roots.handleAt(4))) { + JSString *tmp = ToString(cx, roots.handleAt(4)); if (tmp) filename.encodeLatin1(cx, tmp); } uint32_t lineno; - if (!JS_GetProperty(cx, exnObject, js_lineNumber_str, tvr.handleAt(5)) || - !ToUint32(cx, roots[5], &lineno)) + if (!JS_GetProperty(cx, exnObject, js_lineNumber_str, roots.handleAt(5)) || + !ToUint32(cx, roots.handleAt(5), &lineno)) { lineno = 0; } uint32_t column; - if (!JS_GetProperty(cx, exnObject, js_columnNumber_str, tvr.handleAt(5)) || - !ToUint32(cx, roots[5], &column)) + if (!JS_GetProperty(cx, exnObject, js_columnNumber_str, roots.handleAt(5)) || + !ToUint32(cx, roots.handleAt(5), &column)) { column = 0; } diff --git a/js/src/jsnum.cpp b/js/src/jsnum.cpp index 29e4cdf8b6ff..092eff5f5950 100644 --- a/js/src/jsnum.cpp +++ b/js/src/jsnum.cpp @@ -1534,7 +1534,7 @@ js::ToNumberSlow(JSContext *cx, Value v, double *out) * conversion. Return converted value in *out on success, false on failure. */ JS_PUBLIC_API(bool) -js::ToInt64Slow(JSContext *cx, const Value &v, int64_t *out) +js::ToInt64Slow(JSContext *cx, const HandleValue v, int64_t *out) { JS_ASSERT(!v.isInt32()); double d; @@ -1553,7 +1553,7 @@ js::ToInt64Slow(JSContext *cx, const Value &v, int64_t *out) * conversion. Return converted value in *out on success, false on failure. */ JS_PUBLIC_API(bool) -js::ToUint64Slow(JSContext *cx, const Value &v, uint64_t *out) +js::ToUint64Slow(JSContext *cx, const HandleValue v, uint64_t *out) { JS_ASSERT(!v.isInt32()); double d; @@ -1568,7 +1568,7 @@ js::ToUint64Slow(JSContext *cx, const Value &v, uint64_t *out) } JS_PUBLIC_API(bool) -js::ToInt32Slow(JSContext *cx, const Value &v, int32_t *out) +js::ToInt32Slow(JSContext *cx, const HandleValue v, int32_t *out) { JS_ASSERT(!v.isInt32()); double d; @@ -1583,7 +1583,7 @@ js::ToInt32Slow(JSContext *cx, const Value &v, int32_t *out) } JS_PUBLIC_API(bool) -js::ToUint32Slow(JSContext *cx, const Value &v, uint32_t *out) +js::ToUint32Slow(JSContext *cx, const HandleValue v, uint32_t *out) { JS_ASSERT(!v.isInt32()); double d; @@ -1598,7 +1598,7 @@ js::ToUint32Slow(JSContext *cx, const Value &v, uint32_t *out) } JS_PUBLIC_API(bool) -js::ToUint16Slow(JSContext *cx, const Value &v, uint16_t *out) +js::ToUint16Slow(JSContext *cx, const HandleValue v, uint16_t *out) { JS_ASSERT(!v.isInt32()); double d; diff --git a/js/src/vm/Interpreter.cpp b/js/src/vm/Interpreter.cpp index 2a23b67cb10b..453fd2deb142 100644 --- a/js/src/vm/Interpreter.cpp +++ b/js/src/vm/Interpreter.cpp @@ -1994,9 +1994,9 @@ END_CASE(JSOP_BINDNAME) #define BITWISE_OP(OP) \ JS_BEGIN_MACRO \ int32_t i, j; \ - if (!ToInt32(cx, regs.sp[-2], &i)) \ + if (!ToInt32(cx, regs.stackHandleAt(-2), &i)) \ goto error; \ - if (!ToInt32(cx, regs.sp[-1], &j)) \ + if (!ToInt32(cx, regs.stackHandleAt(-1), &j)) \ goto error; \ i = i OP j; \ regs.sp--; \ @@ -2123,9 +2123,9 @@ END_CASE(JSOP_GE) #define SIGNED_SHIFT_OP(OP) \ JS_BEGIN_MACRO \ int32_t i, j; \ - if (!ToInt32(cx, regs.sp[-2], &i)) \ + if (!ToInt32(cx, regs.stackHandleAt(-2), &i)) \ goto error; \ - if (!ToInt32(cx, regs.sp[-1], &j)) \ + if (!ToInt32(cx, regs.stackHandleAt(-1), &j)) \ goto error; \ i = i OP (j & 31); \ regs.sp--; \ diff --git a/js/src/vm/TypedArrayObject.cpp b/js/src/vm/TypedArrayObject.cpp index b0343ded4225..0a720e7b3cf9 100644 --- a/js/src/vm/TypedArrayObject.cpp +++ b/js/src/vm/TypedArrayObject.cpp @@ -189,7 +189,8 @@ JSBool ArrayBufferObject::class_constructor(JSContext *cx, unsigned argc, Value *vp) { int32_t nbytes = 0; - if (argc > 0 && !ToInt32(cx, vp[2], &nbytes)) + CallArgs args = CallArgsFromVp(argc, vp); + if (argc > 0 && !ToInt32(cx, args[0], &nbytes)) return false; if (nbytes < 0) { @@ -205,7 +206,7 @@ ArrayBufferObject::class_constructor(JSContext *cx, unsigned argc, Value *vp) JSObject *bufobj = create(cx, uint32_t(nbytes)); if (!bufobj) return false; - vp->setObject(*bufobj); + args.rval().setObject(*bufobj); return true; } @@ -1829,30 +1830,29 @@ class TypedArrayObjectTemplate : public TypedArrayObject class_constructor(JSContext *cx, unsigned argc, Value *vp) { /* N.B. this is a constructor for protoClass, not fastClass! */ - JSObject *obj = create(cx, argc, JS_ARGV(cx, vp)); + CallArgs args = CallArgsFromVp(argc, vp); + JSObject *obj = create(cx, args); if (!obj) return false; - vp->setObject(*obj); + args.rval().setObject(*obj); return true; } static JSObject * - create(JSContext *cx, unsigned argc, Value *argv) + create(JSContext *cx, const CallArgs& args) { - /* N.B. there may not be an argv[-2]/argv[-1]. */ - /* () or (number) */ uint32_t len = 0; - if (argc == 0 || ValueIsLength(argv[0], &len)) + if (args.length() == 0 || ValueIsLength(args[0], &len)) return fromLength(cx, len); /* (not an object) */ - if (!argv[0].isObject()) { + if (!args[0].isObject()) { JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_TYPED_ARRAY_BAD_ARGS); return NULL; } - RootedObject dataObj(cx, &argv[0].toObject()); + RootedObject dataObj(cx, &args.get(0).toObject()); /* * (typedArray) @@ -1869,8 +1869,8 @@ class TypedArrayObjectTemplate : public TypedArrayObject int32_t byteOffset = 0; int32_t length = -1; - if (argc > 1) { - if (!ToInt32(cx, argv[1], &byteOffset)) + if (args.length() > 1) { + if (!ToInt32(cx, args[1], &byteOffset)) return NULL; if (byteOffset < 0) { JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, @@ -1878,8 +1878,8 @@ class TypedArrayObjectTemplate : public TypedArrayObject return NULL; } - if (argc > 2) { - if (!ToInt32(cx, argv[2], &length)) + if (args.length() > 2) { + if (!ToInt32(cx, args[2], &length)) return NULL; if (length < 0) { JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,