[JSC] NumberPrototype::extractRadixFromArgs incorrectly cast double to int32_t
authorticaiolima@gmail.com <ticaiolima@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 13 Jan 2018 15:16:23 +0000 (15:16 +0000)
committerticaiolima@gmail.com <ticaiolima@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 13 Jan 2018 15:16:23 +0000 (15:16 +0000)
https://bugs.webkit.org/show_bug.cgi?id=181182

Reviewed by Darin Adler.

JSTests:

* bigIntTests.yaml:
* stress/big-int-constructor.js:
* stress/big-int-prototype-to-string-cast-overflow.js: Added.
(assert):
(assertThrowRangeError):
* stress/number-prototype-to-string-cast-overflow.js: Added.
(assert):
(assertThrowRangeError):

Source/JavaScriptCore:

Casting double to integer is undefined behavior when the truncation
results into a value that doesn't fit into integer size, according C++
spec[1]. Thus, we are changing bigIntProtoFuncToString and
numberProtoFuncToString to remove these source of undefined behavior.

[1] - http://en.cppreference.com/w/cpp/language/implicit_conversion

* runtime/BigIntPrototype.cpp:
(JSC::bigIntProtoFuncToString):
* runtime/NumberPrototype.cpp:
(JSC::numberProtoFuncToString):
(JSC::extractRadixFromArgs): Deleted.
(JSC::extractToStringRadixArgument): Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@226937 268f45cc-cd09-0410-ab3c-d52691b4dbfc

13 files changed:
JSTests/ChangeLog
JSTests/bigIntTests.yaml
JSTests/stress/big-int-constructor.js
JSTests/stress/big-int-prototype-to-string-cast-overflow.js [new file with mode: 0644]
JSTests/stress/big-int-prototype-to-string-exception.js [new file with mode: 0644]
JSTests/stress/big-int-prototype-to-string-wrong-values.js [new file with mode: 0644]
JSTests/stress/number-prototype-to-string-cast-overflow.js [new file with mode: 0644]
JSTests/stress/number-prototype-to-string-exception.js [new file with mode: 0644]
JSTests/stress/number-prototype-to-string-wrong-values.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/BigIntPrototype.cpp
Source/JavaScriptCore/runtime/NumberPrototype.cpp
Source/JavaScriptCore/runtime/NumberPrototype.h

index 190f58ce577a43eda23e7dbde86196c288f15c95..305ac086befcb368338311cdf2d29d81906c2976 100644 (file)
@@ -1,3 +1,19 @@
+2018-01-13  Caio Lima  <ticaiolima@gmail.com>
+
+        [JSC] NumberPrototype::extractRadixFromArgs incorrectly cast double to int32_t
+        https://bugs.webkit.org/show_bug.cgi?id=181182
+
+        Reviewed by Darin Adler.
+
+        * bigIntTests.yaml:
+        * stress/big-int-constructor.js:
+        * stress/big-int-prototype-to-string-cast-overflow.js: Added.
+        (assert):
+        (assertThrowRangeError):
+        * stress/number-prototype-to-string-cast-overflow.js: Added.
+        (assert):
+        (assertThrowRangeError):
+
 2018-01-12  Saam Barati  <sbarati@apple.com>
 
         CheckStructure can be incorrectly subsumed by CheckStructureOrEmpty
index 7ae22cb1bb2aff62cc4ddf496299f96a0744f7ae..cd8636efd1ea6b72ccab3e1fd60a3af9027ee156 100644 (file)
 - path: stress/big-int-to-string.js
   cmd: runBigIntEnabled
 
+- path: stress/big-int-prototype-to-string-cast-overflow.js
+  cmd: runBigIntEnabled
+
+- path: stress/big-int-prototype-to-string-exception.js
+  cmd: runBigIntEnabled
+
+- path: stress/big-int-prototype-to-string-wrong-values.js
+  cmd: runBigIntEnabled
+
index c76843bf82f45eb93ae6711eeb6ea9f1323a119e..4faefbcb3d1791134aa5a9afac18674a29694d9c 100644 (file)
@@ -75,7 +75,7 @@ assert(n.toString() === "15");
 n = BigInt("0b10");
 assert(n.toString() === "2");
 
-n = BigInt("0b10");
+n = BigInt("0b010");
 assert(n.toString() === "2");
 
 let binaryString = "0b1";
@@ -91,7 +91,7 @@ assert(n.toString() === "15");
 n = BigInt("0B10");
 assert(n.toString() === "2");
 
-n = BigInt("0B10");
+n = BigInt("0B010");
 assert(n.toString() === "2");
 
 binaryString = "0B1";
diff --git a/JSTests/stress/big-int-prototype-to-string-cast-overflow.js b/JSTests/stress/big-int-prototype-to-string-cast-overflow.js
new file mode 100644 (file)
index 0000000..a9fa302
--- /dev/null
@@ -0,0 +1,20 @@
+//@ runBigIntEnabled
+
+function assert(a) {
+    if (!a)
+        throw new Error("Bad assertion");
+}
+
+function assertThrowRangeError(input) {
+    try {
+        let number = 3n;
+        number.toString(input);
+        assert(false);
+    } catch (e) {
+        assert(e instanceof RangeError);
+    }
+}
+
+assertThrowRangeError(1e100);
+assertThrowRangeError(-1e101);
+
diff --git a/JSTests/stress/big-int-prototype-to-string-exception.js b/JSTests/stress/big-int-prototype-to-string-exception.js
new file mode 100644 (file)
index 0000000..6c1d3c0
--- /dev/null
@@ -0,0 +1,22 @@
+//@ runBigIntEnabled
+
+function assert(a) {
+    if (!a)
+        throw new Error("Bad assertion");
+}
+
+let o = {
+    valueOf: () => {
+        throw new Error("Bad");
+        return 2;
+    }
+}
+
+let a = 20n;
+try {
+    a.toString(o);
+    assert(false);
+} catch (e) {
+    assert(e.message == "Bad");
+}
+
diff --git a/JSTests/stress/big-int-prototype-to-string-wrong-values.js b/JSTests/stress/big-int-prototype-to-string-wrong-values.js
new file mode 100644 (file)
index 0000000..79e51d5
--- /dev/null
@@ -0,0 +1,31 @@
+//@ runBigIntEnabled
+
+function assert(a) {
+    if (!a)
+        throw new Error("Bad assertion");
+}
+
+function assertRangeError(v) {
+    let a = 456n;
+    try {
+        a.toString(v);
+        assert(false);
+    } catch (e) {
+        assert(e instanceof RangeError);
+    }
+}
+
+assertRangeError(1);
+assertRangeError(37);
+assertRangeError(37.1);
+assertRangeError(37.2);
+assertRangeError(0);
+assertRangeError(-1);
+assertRangeError(1.999999);
+assertRangeError(37.00000000000000001);
+assertRangeError(NaN);
+assertRangeError(null);
+assertRangeError(+Infinity);
+assertRangeError(-Infinity);
+assertRangeError(-0);
+
diff --git a/JSTests/stress/number-prototype-to-string-cast-overflow.js b/JSTests/stress/number-prototype-to-string-cast-overflow.js
new file mode 100644 (file)
index 0000000..f12ab05
--- /dev/null
@@ -0,0 +1,18 @@
+function assert(a) {
+    if (!a)
+        throw new Error("Bad assertion");
+}
+
+function assertThrowRangeError(input) {
+    try {
+        let number = 3;
+        number.toString(input);
+        assert(false);
+    } catch (e) {
+        assert(e instanceof RangeError);
+    }
+}
+
+assertThrowRangeError(1e100);
+assertThrowRangeError(-1e101);
+
diff --git a/JSTests/stress/number-prototype-to-string-exception.js b/JSTests/stress/number-prototype-to-string-exception.js
new file mode 100644 (file)
index 0000000..7445907
--- /dev/null
@@ -0,0 +1,20 @@
+function assert(a) {
+    if (!a)
+        throw new Error("Bad assertion");
+}
+
+let o = {
+    valueOf: () => {
+        throw new Error("Bad");
+        return 2;
+    }
+}
+
+let a = 2;
+try {
+    a.toString(o);
+    assert(false);
+} catch (e) {
+    assert(e.message == "Bad");
+}
+
diff --git a/JSTests/stress/number-prototype-to-string-wrong-values.js b/JSTests/stress/number-prototype-to-string-wrong-values.js
new file mode 100644 (file)
index 0000000..52e6e11
--- /dev/null
@@ -0,0 +1,29 @@
+function assert(a) {
+    if (!a)
+        throw new Error("Bad assertion");
+}
+
+function assertRangeError(v) {
+    let a = 2;
+    try {
+        a.toString(v);
+        assert(false);
+    } catch (e) {
+        assert(e instanceof RangeError);
+    }
+}
+
+assertRangeError(1);
+assertRangeError(37);
+assertRangeError(37.1);
+assertRangeError(37.2);
+assertRangeError(0);
+assertRangeError(-1);
+assertRangeError(1.999999);
+assertRangeError(37.00000000000000001);
+assertRangeError(NaN);
+assertRangeError(null);
+assertRangeError(+Infinity);
+assertRangeError(-Infinity);
+assertRangeError(-0);
+
index 0607a5ed8be132811125e235823bcba24d17b9a7..48966b5eefc92bb0d84a9ffe4c490b7da89631f7 100644 (file)
@@ -1,3 +1,24 @@
+2018-01-13  Caio Lima  <ticaiolima@gmail.com>
+
+        [JSC] NumberPrototype::extractRadixFromArgs incorrectly cast double to int32_t
+        https://bugs.webkit.org/show_bug.cgi?id=181182
+
+        Reviewed by Darin Adler.
+
+        Casting double to integer is undefined behavior when the truncation
+        results into a value that doesn't fit into integer size, according C++
+        spec[1]. Thus, we are changing bigIntProtoFuncToString and
+        numberProtoFuncToString to remove these source of undefined behavior.
+
+        [1] - http://en.cppreference.com/w/cpp/language/implicit_conversion
+
+        * runtime/BigIntPrototype.cpp:
+        (JSC::bigIntProtoFuncToString):
+        * runtime/NumberPrototype.cpp:
+        (JSC::numberProtoFuncToString):
+        (JSC::extractRadixFromArgs): Deleted.
+        (JSC::extractToStringRadixArgument): Added.
+
 2018-01-12  Saam Barati  <sbarati@apple.com>
 
         Move ExitProfile to UnlinkedCodeBlock so it can be shared amongst CodeBlocks backed by the same UnlinkedCodeBlock
index bb903e4be54344df19155527b41dba26c0b41765..19971422448fc1ece65f840339cba3678ae7c2dc 100644 (file)
@@ -35,7 +35,9 @@
 #include "JSFunction.h"
 #include "JSGlobalObject.h"
 #include "JSString.h"
+#include "NumberPrototype.h"
 #include <wtf/Assertions.h>
+#include <wtf/Variant.h>
 
 namespace JSC {
 
@@ -99,21 +101,11 @@ EncodedJSValue JSC_HOST_CALL bigIntProtoFuncToString(ExecState* state)
     
     ASSERT(value);
 
-    int64_t radix;
-    JSValue radixValue = state->argument(0);
-    if (radixValue.isInt32())
-        radix = radixValue.asInt32();
-    else if (radixValue.isUndefined())
-        radix = 10;
-    else {
-        radix = static_cast<int64_t>(radixValue.toInteger(state));
-        RETURN_IF_EXCEPTION(scope, encodedJSValue());
-    }
-
-    if (radix < 2 || radix > 36)
-        return throwVMError(state, scope, createRangeError(state, ASCIILiteral("toString() radix argument must be between 2 and 36")));
+    auto radix = extractToStringRadixArgument(state, state->argument(0));
+    if (WTF::holds_alternative<EncodedJSValue>(radix))
+        return WTF::get<EncodedJSValue>(radix);
 
-    String resultString = value->toString(*state, static_cast<int32_t>(radix));
+    String resultString = value->toString(*state, WTF::get<int32_t>(radix));
     RETURN_IF_EXCEPTION(scope, encodedJSValue());
     if (resultString.length() == 1)
         return JSValue::encode(vm.smallStrings.singleCharacterString(resultString[0]));
index a63d01e4d8bdaf8d9c9a461207b5b42a08745055..01b23421155a10bd995681d29caa35b6e086cea0 100644 (file)
@@ -34,6 +34,7 @@
 #include <wtf/dtoa.h>
 #include <wtf/Assertions.h>
 #include <wtf/MathExtras.h>
+#include <wtf/Variant.h>
 #include <wtf/dtoa/double-conversion.h>
 
 using DoubleToStringConverter = WTF::double_conversion::DoubleToStringConverter;
@@ -504,20 +505,6 @@ EncodedJSValue JSC_HOST_CALL numberProtoFuncToPrecision(ExecState* exec)
     return JSValue::encode(jsString(exec, String(numberToFixedPrecisionString(x, significantFigures, buffer))));
 }
 
-static inline int32_t extractRadixFromArgs(ExecState* exec)
-{
-    JSValue radixValue = exec->argument(0);
-    int32_t radix;
-    if (radixValue.isInt32())
-        radix = radixValue.asInt32();
-    else if (radixValue.isUndefined())
-        radix = 10;
-    else
-        radix = static_cast<int32_t>(radixValue.toInteger(exec)); // nan -> 0
-
-    return radix;
-}
-
 static ALWAYS_INLINE JSString* int32ToStringInternal(VM& vm, int32_t value, int32_t radix)
 {
     ASSERT(!(radix < 2 || radix > 36));
@@ -586,22 +573,21 @@ JSString* numberToString(VM& vm, double doubleValue, int32_t radix)
     return numberToStringInternal(vm, doubleValue, radix);
 }
 
-EncodedJSValue JSC_HOST_CALL numberProtoFuncToString(ExecState* exec)
+EncodedJSValue JSC_HOST_CALL numberProtoFuncToString(ExecState* state)
 {
-    VM& vm = exec->vm();
+    VM& vm = state->vm();
     auto scope = DECLARE_THROW_SCOPE(vm);
 
     double doubleValue;
-    if (!toThisNumber(exec->thisValue(), doubleValue))
-        return throwVMTypeError(exec, scope);
-
-    int32_t radix = extractRadixFromArgs(exec);
-    RETURN_IF_EXCEPTION(scope, encodedJSValue());
-    if (radix < 2 || radix > 36)
-        return throwVMError(exec, scope, createRangeError(exec, ASCIILiteral("toString() radix argument must be between 2 and 36")));
+    if (!toThisNumber(state->thisValue(), doubleValue))
+        return throwVMTypeError(state, scope);
 
     scope.release();
-    return JSValue::encode(numberToStringInternal(vm, doubleValue, radix));
+    auto radix = extractToStringRadixArgument(state, state->argument(0));
+    if (WTF::holds_alternative<EncodedJSValue>(radix))
+        return WTF::get<EncodedJSValue>(radix);
+
+    return JSValue::encode(numberToStringInternal(vm, doubleValue, WTF::get<int32_t>(radix)));
 }
 
 EncodedJSValue JSC_HOST_CALL numberProtoFuncToLocaleString(ExecState* exec)
@@ -628,4 +614,24 @@ EncodedJSValue JSC_HOST_CALL numberProtoFuncValueOf(ExecState* exec)
     return JSValue::encode(jsNumber(x));
 }
 
+Variant<int32_t, EncodedJSValue> extractToStringRadixArgument(ExecState* state, JSValue radixValue)
+{
+    if (radixValue.isUndefined())
+        return 10;
+
+    auto scope = DECLARE_THROW_SCOPE(state->vm());
+    if (radixValue.isInt32()) {
+        int32_t radix = radixValue.asInt32();
+        if (radix >= 2 && radix <= 36)
+            return radix;
+    } else {
+        double radixDouble = radixValue.toInteger(state);
+        RETURN_IF_EXCEPTION(scope, encodedJSValue());
+        if (radixDouble >= 2 && radixDouble <= 36)
+            return static_cast<int32_t>(radixDouble);   
+    }
+
+    return throwVMError(state, scope, createRangeError(state, ASCIILiteral("toString() radix argument must be between 2 and 36")));
+}
+
 } // namespace JSC
index 039b0d59d1ec5bd35a5ac2faf511017aaab58ade..3796187b48d81f939459787c05222a921b7720dd 100644 (file)
@@ -21,6 +21,7 @@
 #pragma once
 
 #include "NumberObject.h"
+#include <wtf/Variant.h>
 
 namespace JSC {
 
@@ -55,5 +56,6 @@ JSString* int32ToString(VM&, int32_t value, int32_t radix);
 JSString* int52ToString(VM&, int64_t value, int32_t radix);
 JSString* numberToString(VM&, double value, int32_t radix);
 String toStringWithRadix(double doubleValue, int32_t radix);
+Variant<int32_t, EncodedJSValue> extractToStringRadixArgument(ExecState*, JSValue radixValue);
 
 } // namespace JSC