Skip to content

Commit 0a9c082

Browse files
ShortDevelopmentShortDevelopment
authored
Force call to ToNumber in Math.max and Math.min (#6851)
According to the spec, Math.max and Math.min should convert all args to numbers (See ToNumber). CC takes a shortcut by just returning NaN as soon as it finds a NaN arg. Therefore, possibly missing calls to valueOf of a later arg. Fix #6519
1 parent a880942 commit 0a9c082

9 files changed

+545
-453
lines changed

‎lib/Runtime/ByteCode/ByteCodeCacheReleaseFileVersion.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
// NOTE: If there is a merge conflict the correct fix is to make a new GUID.
77
// This file was generated with tools/regenByteCode.py
88

9-
// {caa01c1a-6c67-45c2-8c3c-382b84ca34c4}
9+
// {5659e34b-2f03-4880-bed4-7a688cd73df1}
1010
const GUID byteCodeCacheReleaseFileVersion =
11-
{ 0xcaa01c1a, 0x6c67, 0x45c2, {0x8c, 0x3c, 0x38, 0x2b, 0x84, 0xca, 0x34, 0xc4 } };
11+
{ 0x5659e34b, 0x2f03, 0x4880, {0xbe, 0xd4, 0x7a, 0x68, 0x8c, 0xd7, 0x3d, 0xf1 } };
1212

‎lib/Runtime/Library/InJavascript/JsBuiltIn.bc.32b.h

+113-108
Large diffs are not rendered by default.

‎lib/Runtime/Library/InJavascript/JsBuiltIn.bc.64b.h

+113-108
Large diffs are not rendered by default.

‎lib/Runtime/Library/InJavascript/JsBuiltIn.nojit.bc.32b.h

+109-103
Large diffs are not rendered by default.

‎lib/Runtime/Library/InJavascript/JsBuiltIn.nojit.bc.64b.h

+109-103
Large diffs are not rendered by default.

‎lib/Runtime/Library/InJavascript/Math_object.js

+29-21
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
//-------------------------------------------------------------------------------------------------------
22
// Copyright (C) Microsoft. All rights reserved.
3-
// Copyright (c) 2021 ChakraCore Project Contributors. All rights reserved.
3+
// Copyright (c) ChakraCore Project Contributors. All rights reserved.
44
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
55
//-------------------------------------------------------------------------------------------------------
66

@@ -18,13 +18,15 @@
1818
// If no arguments are given, the result is positive infinity
1919
// If any value is NaN, the result is NaN.
2020
// The comparison of values to determine the smallest value is done using the Abstract Relational Comparison algorithm except that +0 is considered to be larger than -0.
21-
if (arguments.length === 0 ) {
21+
if (arguments.length === 0) {
2222
return __chakraLibrary.positiveInfinity;
2323
}
24-
24+
25+
let returnNaN = false;
26+
2527
value1 = +value1;
2628
if (value1 !== value1) {
27-
return NaN;
29+
returnNaN = true;
2830
}
2931

3032
if (arguments.length === 1) {
@@ -33,10 +35,10 @@
3335

3436
if (arguments.length === 2) {
3537
value2 = +value2;
36-
if (value2 !== value2) {
38+
if (value2 !== value2 || returnNaN) {
3739
return NaN;
3840
}
39-
if ((value1 < value2) || (value1 === value2 && value1 === 0 && 1/value1 < 1/value2)) { // checks for -0 and +0
41+
if ((value1 < value2) || (value1 === value2 && value1 === 0 && 1 / value1 < 1 / value2)) { // checks for -0 and +0
4042
return value1;
4143
}
4244
else {
@@ -48,15 +50,17 @@
4850
let nextVal;
4951

5052
for (let i = 1; i < arguments.length; i++) {
51-
nextVal = +arguments[i];
52-
if (nextVal !== nextVal) {
53-
return NaN;
53+
nextVal = +arguments[i]; // Force conversion for all args (ensure call to valueOf)
54+
if (returnNaN) { } // Skip check if possible
55+
else if (nextVal !== nextVal) {
56+
returnNaN = true;
57+
min = NaN;
5458
}
55-
else if ((min > nextVal) || (min === nextVal && min === 0 && 1/min > 1/nextVal)) { // checks for -0 and +0
59+
else if ((min > nextVal) || (min === nextVal && min === 0 && 1 / min > 1 / nextVal)) { // checks for -0 and +0
5660
min = nextVal;
5761
}
5862
}
59-
63+
6064
return min;
6165
});
6266

@@ -69,10 +73,12 @@
6973
if (arguments.length === 0) {
7074
return __chakraLibrary.negativeInfinity;
7175
}
72-
76+
77+
let returnNaN = false;
78+
7379
value1 = +value1;
7480
if (value1 !== value1) {
75-
return NaN;
81+
returnNaN = true;
7682
}
7783

7884
if (arguments.length === 1) {
@@ -81,10 +87,10 @@
8187

8288
if (arguments.length === 2) {
8389
value2 = +value2;
84-
if (value2 !== value2) {
90+
if (value2 !== value2 || returnNaN) {
8591
return NaN;
8692
}
87-
if ((value1 > value2) || (value1 === value2 && value1 === 0 && 1/value1 > 1/value2)) { // checks for -0 and +0
93+
if ((value1 > value2) || (value1 === value2 && value1 === 0 && 1 / value1 > 1 / value2)) { // checks for -0 and +0
8894
return value1;
8995
}
9096
else {
@@ -96,15 +102,17 @@
96102
let nextVal;
97103

98104
for (let i = 1; i < arguments.length; i++) {
99-
nextVal = +arguments[i];
100-
if (nextVal !== nextVal) {
101-
return NaN;
105+
nextVal = +arguments[i]; // Force conversion for all args (ensure call to valueOf)
106+
if (returnNaN) { } // Skip check if possible
107+
else if (nextVal !== nextVal) {
108+
returnNaN = true;
109+
max = NaN;
102110
}
103-
else if ((max < nextVal) || (max === nextVal && max === 0 && 1/max < 1/nextVal)) { // checks for -0 and +0
111+
else if ((max < nextVal) || (max === nextVal && max === 0 && 1 / max < 1 / nextVal)) { // checks for -0 and +0
104112
max = nextVal;
105-
}
113+
}
106114
}
107-
115+
108116
return max;
109117
});
110118
});

‎lib/Runtime/Library/MathLibrary.cpp

+23-8
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
//-------------------------------------------------------------------------------------------------------
22
// Copyright (C) Microsoft. All rights reserved.
3+
// Copyright (c) ChakraCore Project Contributors. All rights reserved.
34
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
45
//-------------------------------------------------------------------------------------------------------
56
#include "RuntimeLibraryPch.h"
@@ -737,28 +738,35 @@ namespace Js
737738
else
738739
{
739740
double current = JavascriptConversion::ToNumber(args[1], scriptContext);
741+
742+
bool returnNaN = false;
740743
if (JavascriptNumber::IsNan(current))
741744
{
742-
return scriptContext->GetLibrary()->GetNaN();
745+
returnNaN = true;
743746
}
744747

745748
for (uint idxArg = 2; idxArg < args.Info.Count; idxArg++)
746749
{
747750
double compare = JavascriptConversion::ToNumber(args[idxArg], scriptContext);
748-
if (JavascriptNumber::IsNan(compare))
751+
if (JavascriptNumber::IsNan(compare) || returnNaN) // Call ToNumber for all args
749752
{
750-
return scriptContext->GetLibrary()->GetNaN();
753+
returnNaN = true;
751754
}
752755

753756
// In C++, -0.0f == 0.0f; however, in ES, -0.0f < 0.0f. Thus, use additional library
754757
// call to test this comparison.
755-
if ((compare == 0 && JavascriptNumber::IsNegZero(current)) ||
758+
else if ((compare == 0 && JavascriptNumber::IsNegZero(current)) ||
756759
current < compare)
757760
{
758761
current = compare;
759762
}
760763
}
761764

765+
if (returnNaN)
766+
{
767+
return scriptContext->GetLibrary()->GetNaN();
768+
}
769+
762770
return JavascriptNumber::ToVarNoCheck(current, scriptContext);
763771
}
764772
}
@@ -817,28 +825,35 @@ namespace Js
817825
else
818826
{
819827
double current = JavascriptConversion::ToNumber(args[1], scriptContext);
828+
829+
bool returnNaN = false;
820830
if (JavascriptNumber::IsNan(current))
821831
{
822-
return scriptContext->GetLibrary()->GetNaN();
832+
returnNaN = true;
823833
}
824834

825835
for (uint idxArg = 2; idxArg < args.Info.Count; idxArg++)
826836
{
827837
double compare = JavascriptConversion::ToNumber(args[idxArg], scriptContext);
828-
if (JavascriptNumber::IsNan(compare))
838+
if (JavascriptNumber::IsNan(compare) || returnNaN) // Call ToNumber for all args
829839
{
830-
return scriptContext->GetLibrary()->GetNaN();
840+
returnNaN = true;
831841
}
832842

833843
// In C++, -0.0f == 0.0f; however, in ES, -0.0f < 0.0f. Thus, use additional library
834844
// call to test this comparison.
835-
if ((current == 0 && JavascriptNumber::IsNegZero(compare)) ||
845+
else if ((current == 0 && JavascriptNumber::IsNegZero(compare)) ||
836846
current > compare)
837847
{
838848
current = compare;
839849
}
840850
}
841851

852+
if (returnNaN)
853+
{
854+
return scriptContext->GetLibrary()->GetNaN();
855+
}
856+
842857
return JavascriptNumber::ToVarNoCheck(current, scriptContext);
843858
}
844859
}

‎test/Math/max2.js

+36
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
//-------------------------------------------------------------------------------------------------------
2+
// Copyright (C) Microsoft. All rights reserved.
3+
// Copyright (c) ChakraCore Project Contributors. All rights reserved.
4+
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
5+
//-------------------------------------------------------------------------------------------------------
6+
7+
// @ts-check
8+
9+
function AssertNaN(value) {
10+
if (!isNaN(value))
11+
throw new Error("Expected NaN as value");
12+
}
13+
14+
let valueOfCounter = 0;
15+
const obj = {
16+
valueOf: function () {
17+
valueOfCounter++;
18+
return 1;
19+
}
20+
};
21+
22+
AssertNaN(Math.max(NaN, obj));
23+
AssertNaN(Math.max(NaN, NaN));
24+
AssertNaN(Math.max(NaN, NaN, obj));
25+
AssertNaN(Math.max(NaN, NaN, NaN));
26+
27+
AssertNaN(Math.min(NaN, obj));
28+
AssertNaN(Math.min(NaN, NaN));
29+
AssertNaN(Math.min(NaN, NaN, obj));
30+
AssertNaN(Math.min(NaN, NaN, NaN));
31+
32+
const expectedCount = 4;
33+
if (valueOfCounter != expectedCount)
34+
throw new Error(`Expected "valueOf" to be called ${expectedCount}x; got ${valueOfCounter}`);
35+
36+
console.log("pass");

‎test/Math/rlexe.xml

+11
Original file line numberDiff line numberDiff line change
@@ -42,4 +42,15 @@
4242
<baseline>clz32.baseline</baseline>
4343
</default>
4444
</test>
45+
<test>
46+
<default>
47+
<files>max2.js</files>
48+
</default>
49+
</test>
50+
<test>
51+
<default>
52+
<files>max2.js</files>
53+
<compile-flags>-JsBuiltin-</compile-flags>
54+
</default>
55+
</test>
4556
</regress-exe>

0 commit comments

Comments
 (0)