Potentially critical bug: Unexpected, reproducible calculation error

[psorowka]

• Version: 8.x
• Platform: docker ( official node as well as mhart/alpine-node)
• Subsystem: core

The following minimal sample code results in reproducible, unexpected behavior:

const one = 150
const two = 2
let counter = 0

setInterval(() => {
  const res = Math.max(5, Math.floor(one / two))
  if (res > 100) {
    console.log(counter, res)
  } else {
    counter++
  }
}, 1)

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 the Math.max, the result of Math.floor looks good.

Side notes

• reliably happens when using node:8, node:8.9, node:8.11, mhart/alpine-node:8.9 (and more) docker images, both on Mac and Linux host systems
• doesn't happen when changing the setInterval to a while loop
• doesn't happen when using node:6 or node:10 docker images
• bug first occured within a big software project and was easily reproducible in this minimal sample
• the counter isn't needed for showing the bug, it's just to demonstrate how deterministic the problem seems to be
• even when splitting the calculation into

const floored = Math.floor(one / two)
const res = Math.max(5, floored)

res will carry the value of one after around 10.000 iterations

I don't get what happens under the hood, but I assume this is critical.

[vsemozhetbyt]

Can reproduce on Windows 7 x64 with Node.js 8.11.4

[BridgeAR]

@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?

[mscdex]

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()

[mscdex]

Also does not seem to reproduce with --minimal which always uses Ignition and performs no optimizations.

[mscdex]

It looks like this was fixed between V8 6.6.281 and 6.6.285.

[mscdex]

Narrowing it down further it seems this commit fixed the issue.

[psorowka]

Interesting. For completeness I'd like to add that I can only reproduce the problem with Math.floor and not e.g. with Math.round. However, the behavior is the same when doing Math.min instead, but the winner of the min/max operation must point to the result of floor, e.g.

Math.min(Math.floor(one/two), 5e5)

[ryzokuken]

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 with 75 .

[psorowka]

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 with 75 .

Right, It is the combination of both (see my last comment), meanwhile I also believe that Math.floor has the error, but it only appears when you refer to it in the Math.max....

[ryzokuken]

@psorowka interesting. I'll try to replicate this on v8's master today and dive deeper if it still exists, thanks to @mscdex narrowing it down to turbofan.

Could someone try this on node's master branch as well?

[hashseed]

@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?

[sigurdschneider]

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?

[hashseed]

@targos did you float this patch? Can this issue be closed?

[targos]

I don't think I did.

[apapirovski]

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.

[apapirovski]

Nope. :( Appears this is still an issue. Maybe ping @targos?

[targos]

working on it

[targos]

Backport: #27358

[BridgeAR]

This was fixed in v8.16.2 by 37e24b1