debugger: guard against call from non-node context by bnoordhuis · Pull Request #4328 · nodejs/node · GitHub

Fix a segmentation fault when the debug message handler was called from
a context without an associated node::Environment.

Fixes: #4261
Fixes: #4322

R=@indutny?

CI: https://ci.nodejs.org/job/node-test-pull-request/1017/

Fix a segmentation fault when the debug message handler was called from a context without an associated `node::Environment`. Fixes: nodejs#4261 Fixes: nodejs#4322

CI looks happy minus a flaky ubuntu test.

LGTM

Fix a segmentation fault when the debug message handler was called from a context without an associated `node::Environment`. Fixes: #4261 Fixes: #4322 PR-URL: #4328 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Landed in 25776f3

Belated LGTM

Fix a segmentation fault when the debug message handler was called from a context without an associated `node::Environment`. Fixes: nodejs#4261 Fixes: nodejs#4322 PR-URL: nodejs#4328 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Is this critical enough to be rushed into 4.2.5?

/cc @jasnell @bnoordhuis

FWIW, the test is flaky on Windows 10 in CI. Not sure why or what the fix might be. See #4343.

The change itself is low risk and quite a few people reported it so I'd say it's a good candidate for LTS. I'll take a look at #4343.

Fix a segmentation fault when the debug message handler was called from a context without an associated `node::Environment`. Fixes: #4261 Fixes: #4322 PR-URL: #4328 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com>

I believe I am seeing this crash in 4.3.0:

PID 33 received SIGSEGV for address: 0x3a8
/usr/local/lib/node_modules/segfault-handler/build/Release/segfault-handler.node(+0x1af4)[0x7fae9c2dfaf4]
/lib/x86_64-linux-gnu/libpthread.so.0(+0x10340)[0x7faea00be340]
node(_ZN4node8debugger5Agent14MessageHandlerERKN2v85Debug7MessageE+0x41)[0xddf351]
node(_ZN2v88internal5Debug20NotifyMessageHandlerENS_10DebugEventENS0_6HandleINS0_8JSObjectEEES5_b+0x7d0)[0xa3a850]
node(_ZN2v88internal5Debug17ProcessDebugEventENS_10DebugEventENS0_6HandleINS0_8JSObjectEEEb+0x93)[0xa3ae43]
node(_ZN2v88internal5Debug14OnAfterCompileENS0_6HandleINS0_6ScriptEEE+0x334)[0xa3c754]
node[0xa1aad8]
node(_ZN2v88internal8Compiler13CompileScriptENS0_6HandleINS0_6StringEEENS2_INS0_6ObjectEEEiiNS_19ScriptOriginOptionsES6_NS2_INS0_7ContextEEEPNS_9ExtensionEPPNS0_10ScriptDataENS_14ScriptCompiler14CompileOptionsENS0_11NativesFlagEb+0x4ce)[0xa1deae]
node(_ZN2v814ScriptCompiler22CompileUnboundInternalEPNS_7IsolateEPNS0_6SourceENS0_14CompileOptionsEb+0x1ef)[0x8dd26f]
node(_ZN2v814ScriptCompiler14CompileUnboundEPNS_7IsolateEPNS0_6SourceENS0_14CompileOptionsE+0xb)[0x8dd43b]
node(_ZN4node16ContextifyScript3NewERKN2v820FunctionCallbackInfoINS1_5ValueEEE+0x5b4)[0xe0b614]
node(_ZN2v88internal25FunctionCallbackArguments4CallEPFvRKNS_20FunctionCallbackInfoINS_5ValueEEEE+0x92)[0x904ea2]
node[0x92e6b2]
[0x2a494db0963b]

Any expected version that a patch will land in?

This is concerning to me since it is blocking my upgrade to 4.3.0, which is recommended as a security upgrade.

@andyburke I totally understand if you want to stick with LTS release and not stable release, but if that doesn't make a difference to you, I believe this is fixed is in 5.4.0 and up.

Is this the only place env could be null?

@andyburke ... since 4.3.0 was a security release all other commits were bumped to the next round. We'll be preparing another LTS update in the coming week or two. We'll try to get this into that batch.

@Trott thanks, but yes, would like to stick with LTS release.

@jasnell hmm, I am trying to upgrade from 4.2.3, so was this issue somehow introduced in the interim? I understand bumping the commits and am glad this will land relatively soon, but shouldn't this have been pretty high priority as a bug/regression?

@andyburke This bug exists in 4.2.3 (and every other 4.x version of Node).

$ node -v
v4.2.3
$ node test/parallel/test-debug-no-context.js 

assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: null === 0
    at ChildProcess.<anonymous> (/Users/trott/io.js/test/parallel/test-debug-no-context.js:15:10)
    at ChildProcess.<anonymous> (/Users/trott/io.js/test/common.js:395:15)
    at emitTwo (events.js:87:13)
    at ChildProcess.emit (events.js:172:7)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:200:12)
$

Somehow I am not seeing this in 4.2.3. Rolling my docker container back to 4.2.3 alleviates this issue. Perhaps there is some module interaction that is different, but rolling back fixed this for me.

Interesting. Ok. I can understand the frustration @andyburke. The original plan had been to get a regular 4.x update out this week but the security release came up and pushed the schedule off just a bit. I'm going to be spinning up work on the next 4.x maintenance release (which is actually going to be quite large I'm afraid, there are quite a few stacked up commits) later on this week. We're going to have a Release Candidate cycle on this one so once the first RC is available let's see if the issue is resolved for you.

Fix a segmentation fault when the debug message handler was called from a context without an associated `node::Environment`. Fixes: #4261 Fixes: #4322 PR-URL: #4328 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Fix a segmentation fault when the debug message handler was called from a context without an associated `node::Environment`. Fixes: nodejs#4261 Fixes: nodejs#4322 PR-URL: nodejs#4328 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * buffer: make byteLength work with Buffer correctly (Jackson Tian) - nodejs#4738 * debugger: guard against call from non-node context (Ben Noordhuis) - nodejs#4328 * node_contextify: do not incept debug context (Myles Borins) - nodejs#4815

Fix a segmentation fault when the debug message handler was called from a context without an associated `node::Environment`. Fixes: nodejs#4261 Fixes: nodejs#4322 PR-URL: nodejs#4328 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * buffer: make byteLength work with Buffer correctly (Jackson Tian) - nodejs#4738 * debugger: guard against call from non-node context (Ben Noordhuis) - nodejs#4328 * node_contextify: do not incept debug context (Myles Borins) - nodejs#4819 * deps: update to http-parser 2.5.2 (James Snell) - nodejs#5238

Notable changes: * buffer: make byteLength work with Buffer correctly (Jackson Tian) - #4738 * debugger: guard against call from non-node context (Ben Noordhuis) - #4328 * node_contextify: do not incept debug context (Myles Borins) - #4819 * deps: update to http-parser 2.5.2 (James Snell) - #5238 PR-URL: #5200 (comment)

Fix a segmentation fault when the debug message handler was called from a context without an associated `node::Environment`. Fixes: nodejs#4261 Fixes: nodejs#4322 PR-URL: nodejs#4328 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com>

debugger: guard against call from non-node context

b452b7f

Fix a segmentation fault when the debug message handler was called from a context without an associated `node::Environment`. Fixes: nodejs#4261 Fixes: nodejs#4322

bnoordhuis added the debugger label Dec 17, 2015

This was referenced Dec 17, 2015

Easy segmentation fault in mac node --debug #4322

Closed

node js 5.2.0 segmentation fault after console.log #4261

Closed

jasnell closed this Dec 17, 2015

jasnell mentioned this pull request Dec 17, 2015

Release 4.2.4 Planning #4321

Closed

bnoordhuis deleted the fix4261 branch December 17, 2015 19:06

bnoordhuis added the lts-watch-v4.x label Dec 17, 2015

mscdex mentioned this pull request Jan 8, 2016

Segmentation fault: 11 when debug Promise #4577

Closed

dan-tull mentioned this pull request Jan 26, 2016

Debugger protocol hangs in some special situations #4597

Closed

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jan 26, 2016

This was referenced Jan 27, 2016

stepping through a three lines of code crashes node 4.2.6 #4917

Closed

received SIGSEGV for address: 0x3a8 , Many times #4908

Closed

MylesBorins mentioned this pull request Feb 11, 2016

V4.3.1 proposal #5200

Merged

weinand mentioned this pull request Apr 5, 2016

Support inspecting large data structures on 4.x and 5.x versions of node.js microsoft/vscode#4951

Closed

3 tasks

snyk-bot mentioned this pull request Feb 12, 2022

[Snyk] Upgrade rollup from 2.58.0 to 2.65.0 Iceymann18777/node#12

Open

leonardoadame mentioned this pull request Dec 1, 2023

[Snyk] Fix for 1 vulnerabilities leonardoadame/node#377

Open

aliscco mentioned this pull request Dec 2, 2023

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#1015

Open

leonardoadame mentioned this pull request Feb 11, 2024

[Snyk] Security upgrade webpack-dev-server from 3.11.3 to 4.8.0 leonardoadame/node#646

Open

aliscco mentioned this pull request Feb 12, 2024

[Snyk] Security upgrade webpack-dev-server from 3.11.3 to 4.8.0 aliscco/alisco-node#1250

Open

debugger: guard against call from non-node context by bnoordhuis · Pull Request #4328 · nodejs/node · GitHub

Uh oh!

Uh oh!

debugger: guard against call from non-node context - #4328

debugger: guard against call from non-node context#4328bnoordhuis wants to merge 1 commit intonodejs:masternodejs/node:masterfrom bnoordhuis:fix4261bnoordhuis/io.js:fix4261Copy head branch name to clipboard

Conversation

bnoordhuis commented Dec 17, 2015

Uh oh!

evanlucas commented Dec 17, 2015

Uh oh!

jasnell commented Dec 17, 2015

Uh oh!

jasnell commented Dec 17, 2015

Uh oh!

indutny commented Dec 17, 2015

Uh oh!

MylesBorins commented Jan 7, 2016

Uh oh!

Trott commented Jan 7, 2016

Uh oh!

bnoordhuis commented Jan 7, 2016

Uh oh!

andyburke commented Feb 10, 2016

Uh oh!

Trott commented Feb 10, 2016

Uh oh!

thefourtheye commented Feb 10, 2016

Uh oh!

jasnell commented Feb 10, 2016

Uh oh!

andyburke commented Feb 10, 2016

Uh oh!

Trott commented Feb 10, 2016

Uh oh!

andyburke commented Feb 10, 2016

Uh oh!

jasnell commented Feb 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

debugger: guard against call from non-node context#4328
bnoordhuis wants to merge 1 commit into
nodejs:masterfrom
bnoordhuis:fix4261