Potentially critical bug: Unexpected, reproducible calculation error #22810
Description
Activity
Can reproduce on Windows 7 x64 with Node.js 8.11.4
@nodejs/v8 this seems like a fixed V8 issue. Since this is pretty bad, could you point out what commit fixed this so we can backport that?
It looks like this is limited to reuse of the same function. Also FWIW I can reproduce this faster by placing the code in a function and calling the function in an infinite while loop:
const one = 150 const two = 2 let counter = 0 function next() { const res = Math.max(5, Math.floor(one / two)) if (res > 100) { console.log(counter, res) } else { counter++ } } while (true) next()
Also does not seem to reproduce with
--minimalwhich always uses Ignition and performs no optimizations.It looks like this was fixed between V8 6.6.281 and 6.6.285.
Narrowing it down further it seems this commit fixed the issue.
Interesting. For completeness I'd like to add that I can only reproduce the problem with
Math.floorand not e.g. withMath.round. However, the behavior is the same when doingMath.mininstead, but the winner of themin/maxoperation must point to the result offloor, e.g.Math.min(Math.floor(one/two), 5e5)The actual bug seems to happen within the Math.max, the result of Math.floor looks good.
Does it? I mean, I couldn't reproduce this after replacing the
Math.floor(...)call with75.The actual bug seems to happen within the Math.max, the result of Math.floor looks good.
Does it? I mean, I couldn't reproduce this after replacing the
Math.floor(...)call with75.Right, It is the combination of both (see my last comment), meanwhile I also believe that
Math.floorhas the error, but it only appears when you refer to it in theMath.max....@sigurdschneider @bmeurer is this an accidental fix for this issue? If yes, could you please spend some time to make sure this is actually fixed?
I've looked into this, and the CL you bisected to only incidentally fixes this example. The real fix is
https://chromium.googlesource.com/v8/v8/+/d520ebb9a85b73b2a6505e133a7cc940c7d2adbd
which should be floated on top of all affected node versions. @targos Could you take care of this?
I don't think I did.
I can no longer reproduce this on node 8.x so I assume the patch was floated and this can be closed out. Please feel free to re-open if you believe I've made a mistake.
Nope. :( Appears this is still an issue. Maybe ping @targos?
working on it
- added a commit that references this issue
on Sep 19, 2019
The following minimal sample code results in reproducible, unexpected behavior:
Expected behavior
This code should never output anything, because the calculation result is always 75
Actual behavior
After a while, the code starts to output the value of the variable
one. This happens reliably once at counter value 5.398 and at each iteration starting from 10.794. The behavior is identical (even more or less the counter values) when changing the values of the variables or making the timer slower. The actual bug seems to happen within theMath.max, the result ofMath.floorlooks good.Side notes
node:8,node:8.9,node:8.11,mhart/alpine-node:8.9(and more) docker images, both on Mac and Linux host systemssetIntervalto awhile loopnode:6ornode:10docker imagesreswill carry the value ofoneafter around 10.000 iterationsI don't get what happens under the hood, but I assume this is critical.