An issue about the Math.max().

[NWU-NISL]

When I passed NaN and an object with the "valueOf" attribute value as a callable function to the first and second parameters of Math.max, chakra did not execute this function. According to the ES10 standard, the ToNumber operation is performed on each parameter of Math.max, and the "valueOf" attribute value function of the second parameter will be executed.

version

chakra-1_11_22

command

ChakraCore/out/Debug/ch testcase.js

testcase

var NISLFuzzingFunc = function(){
    var a = {
        valueOf: function(){
            print(1);
        }
    };
    var b = Math.max(NaN,a);
};
NISLFuzzingFunc();

Output

No output.

Expected behavior

Output 1.

Contributor : @Haobin-Lee

[OmarMorales71]

@ppenzin Hello, I would like to contribute to this project solving this issue. I'm working on it :D

[OmarMorales71]

@ppenzin Where I can fine the definition of the Math.max() method?

[ppenzin]

@OmarMorales71 thank you for your interest! This is the most complete spec forMath.max():

https://tc39.es/ecma262/#sec-math.max

Feel free to ask if you have any further questions!

[rhuanjl]

Interestingly if you reverse the order of parameters Math.max(a,NaN) you get the expected output.

Also noted that this repros in both Master and release 1.11.

The implementation of Math.max was changed between the two, in 1.11 it's implemented here:
https://github.com/microsoft/ChakraCore/blob/d8cbaff6a2ef6750a51518c560644f6b64fce928/lib/Runtime/Library/MathLibrary.cpp#L694-L764

In master it's instead implemented with self-hosted JS here:
https://github.com/microsoft/ChakraCore/blob/d8cbaff6a2ef6750a51518c560644f6b64fce928/lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js#L794-L841

The 1.11 C++ implementation is preserved in master as a fallback in case self-hosted JS is unavailable for some reason. It's interesting that both implementations reproduce this error.

@OmarMorales71 Looking at the JS implementation, to fix the error we unfortunately need to slightly de-optimise the toNumber step is done by doing value = +value we need to do that to every parameter before we do anything that can return - try to figure out how to do that without any unnecessary steps - also we want to avoid accessing indexed argument values in the cases where we don't need to. We should also fix the C++ implementation - it will probably be a similar issue.
Note if you fix the JS implementation you'll need to run the bytecode regen before then building to test it either test/xplatregenbytecode.py (on linux or macOS) OR regenallbytecode.cmd on windows

[Abdullah17m]

Hello there i am new to open source contribution. Where can i find this piece of code in your repository?

[rhuanjl]

Hello there i am new to open source contribution. Where can i find this piece of code in your repository?

Have a look at the links in my previous comment should take you to the correct source files.

[anshul2807]

Hey I want to Contribute on this Issue, Can I??

[rhuanjl]

@anshul2807 it's a good first issue if you want to take it - though not sure if @Abdullah17m was already intending to take it?

Don't want two people working on the same issue.

[ayushrana86]

Hello, @rhuanjl is this issue still available.
I tried creating this error by reversing the order of parameters Math.max(a,NaN) but the results I got is NaN can put a snapshot of the result that you got, it will help a lot.

[rhuanjl]

Hello, @rhuanjl is this issue still available.

I tried creating this error by reversing the order of parameters Math.max(a,NaN) but the results I got is NaN can put a snapshot of the result that you got, it will help a lot.

The issue is still available, the error is that the function short cuts/does an early return of the first parameter is NaN.

[YaseenYo]

Hello, @rhuanjl can we solve this issue by adding a for loop before considering the first argument...

//can we add this, It just iterates over all the arguments
for (let i = 1; i < arguments.length; i++) { nextVal = +arguments[i]; }

//already in code
value1 = +value1; if (value1 !== value1) { return NaN; }

[YaseenYo]

@rhuanjl Can We do this?

    let max = value1;
    let nextVal;
    let isNaN = false;

    for (let i = 1; i < arguments.length; i++) {
        nextVal = +arguments[i];
        if (nextVal !== nextVal) {
            isNaN = true;
        }
        else if(isNaN){
             continue;
        }
        else if ((max < nextVal) || (max === nextVal && max === 0 && 1/max < 1/nextVal)) { 
            max = nextVal;
        } 
    }

    if(isNaN){
         return NaN;
    }
    else {
         return max;
    }

[Harsh-Shedge]

Is this issue resolved? Can I try to work on this?

[ppenzin]

@Harsh-Shedge sure, you can give it a try.