Node: segfault if clearTimeout(interval)

[watson]

• Version: v6.9.1
• Platform: Darwin watson-3.local 16.1.0 Darwin Kernel Version 16.1.0: Thu Oct 13 21:26:57 PDT 2016; root:xnu-3789.21.3~60/RELEASE_X86_64 x86_64
• Subsystem:

If using clearTimeout to clear an unreffed Timeout object returned by setInterval(...).unref() you'll get a segfault the next time the interval would normally have fired.

The following example program will segfault after 4 seconds:

// keep the event loop busy while we wait for segfault
setTimeout(function () {}, 100000)

console.log('setInterval')
var timer = setInterval(clear, 2000).unref()

function clear () {
  console.log('clear - start')

  // Use clearTimeout instead of clearInterval
  clearTimeout(timer)

  console.log('clear - end')
}

The segfault doesn't happen if the Timeout object isn't unreffed.

Even though the user should just use clearInterval instead, this at least shouldn't segfault.

[bnoordhuis]

I did a quick bisect to see if it's a regression. I didn't find a culprit but it seems to go back to July at least.

EDIT: Worth noting that the crash seems to be caused by a wrap->object() in OnTimeout() that is a tagged integer, not a JS heap object.

[Fishrock123]

Worth noting that the crash seems to be caused by a wrap->object() in OnTimeout() that is a tagged integer, not a JS heap object.

Are you sure about that? The unsigned integer should be the property name, which it appears to be?

[Fishrock123]

Ok, I think rearm() is just not properly checking if the timer was unenrolled.

[Fishrock123]

Not enough care was given in the recent interval refactor and several related bugs also exist. The following do not work correctly inside an Interval callback:

• clearTimeout(timer)
• Timer#close()
• unenroll(timer)
• timer._onTimeout = null
• timer._idleTimeout = -1

Edit: Some of these may have existed before the refactor, so it is probably not entirely to blame.

[MylesBorins]

@Fishrock123 can you link to the PR of the interval refactor please

[Fishrock123]

@TheAlphaNerd sorry, was short on time earlier.

Commit: https://github.com/nodejs/node/commit/c8c2544cd9c339cdde881fc9a7f0851971b94d72
Pull request: https://github.com/nodejs/node/pull/8661

[Fishrock123]

This is in part due to the fault of clarity in the timers codebase that _idleTimeout === -1 is the closest to an authoritative way to tell if a timer is canceled, but that isn't 100% maintained in every possible cancel path. (I will probably fix _some_ of this at the same time.)

[Fishrock123]

Hmmm, actually fixing this correctly is proving a bit of a challenge, there are some odd cases with Timeout#close() when unrefed.

[Fishrock123]

Still on this, will try to fix tomorrow or next week.

[jucrouzet]

If it can help, also crashes on Node 4.4.2 with a more verbose message :
node: ../src/timer_wrap.cc:74: static void node::TimerWrap::Start(const v8::FunctionCallbackInfo<v8::Value>&): AssertionHandleWrap::IsAlive(wrap)' failed.`