Windows ConPTY backend data race in ptyHandles can crash in remove_pty_baton

[stephentoub]

The Windows ConPTY backend appears to have a data race around the global ptyHandles vector in src/win/conpty.cc.

ptyHandles is a process-global std::vector<std::unique_ptr<pty_baton>>. It is accessed from multiple threads without synchronization:

• the JS/main thread inserts into it in PtyStartProcess
• the JS/main thread reads from it via get_pty_baton in connect, resize, clear, and kill
• each PTY process gets a native watcher thread from SetupExitCallback
• each watcher thread calls remove_pty_baton(baton->id) when its child process exits

The problematic code is:

static std::vector<std::unique_ptr<pty_baton>> ptyHandles;

static bool remove_pty_baton(int id) {
  auto it = std::remove_if(ptyHandles.begin(), ptyHandles.end(), [id](const auto& ptyHandle) {
    return ptyHandle->id == id;
  });
  if (it != ptyHandles.end()) {
    ptyHandles.erase(it);
    return true;
  }
  return false;
}

Because multiple SetupExitCallback watcher threads can call remove_pty_baton concurrently, two std::remove_if calls can race while moving/erasing unique_ptr elements in the same vector. One thread can observe an element that has been moved from by another thread and then dereference a null unique_ptr in the predicate (ptyHandle->id). There are also races between get_pty_baton and removal/erase.

A crash dump from a release build showed two native threads simultaneously inside remove_pty_baton, both from SetupExitCallback watcher lambdas. The faulting thread crashed at the predicate dereference with rax == 0:

conpty!remove_pty_baton::<lambda>::operator()+0x3
cmp dword ptr [rax], ebx
Exception code: 0xc0000005
Attempt to read from address 0000000000000000

This is consistent with ptyHandle being a moved-from/null std::unique_ptr during concurrent std::remove_if.

[anthonykim1]

@deepak1556 Should we lock the shared list and/or switch to something like: std::shared_ptr<pty_baton> possibly?

There was also concern where asserts were showing up in release builds and I think its from:

node-pty/src/win/conpty.cc

Line 106 in 21b81ac

assert(remove_pty_baton(baton->id));

[deepak1556]

Should we lock the shared list and/or switch to something like: std::shared_ptr<pty_baton> possibly?

This can be addressed with an exclusive access to the global, using shared_ptr will just make the lifetime harder to reason for no advantage in this case.

There was also concern where asserts were showing up in release builds and I think its from:

Thats something the addon inherits from Node.js build configuration

[deepak1556]

Thanks for capturing this issue @stephentoub

[StephanTLavavej]

@anthonykim1

There was also concern where asserts were showing up in release builds

@deepak1556

Thats something the addon inherits from Node.js build configuration

Are you sure it's not controlled by this repo's build configuration? I saw

node-pty/binding.gyp

Lines 7 to 32 in fa83ecf

	['OS=="win"', {
	'msvs_configuration_attributes': {
	'SpectreMitigation': 'Spectre'
	},
	'msvs_settings': {
	'VCCLCompilerTool': {
	'AdditionalOptions': [
	'/guard:cf',
	'/sdl',
	'/W3',
	'/we4146',
	'/we4244',
	'/we4267',
	'/ZH:SHA_256'
	]
	},
	'VCLinkerTool': {
	'AdditionalOptions': [
	'/DYNAMICBASE',
	'/guard:cf'
	]
	}
	},
	}, {
	'cflags': ['-O2', '-fstack-protector-strong'],
	}],

where it looks like you're passing -O2 to request optimizations, but there's no -DNDEBUG to control <assert.h>'s behavior.

Separately, I don't really know the format of this weird file, but it looks like it might be sending -fstack-protector-strong to MSVC, which will be ignored:

C:\Temp>cl /EHsc /nologo /W4 -O2 -fstack-protector-strong meow.cpp
cl : Command line warning D9002 : ignoring unknown option '-fstack-protector-strong'
meow.cpp

C:\Temp>

[deepak1556]

but there's no -DNDEBUG to control <assert.h>'s behavior.

What I mean is when building the addon a hidden details is that, we inherit a base configuration from Node.js/Electron https://github.com/nodejs/node/blob/bf7e79c264eef9bd0c743588aa6a158a26d90103/common.gypi#L180-L275 the binary into which the addon gets loaded which sets the minimum configuration/expectation (/MT or /MD for CRT) etc. What we have in the addon gyp are additions that were only required because of binskim policies. Ideally an addon doesn't need any special release configuration tweaks. We can define NDEBUG in the addon gyp, but I don't do it since the binary itself doesn't build with it and to avoid possible ODR.

Separately, I don't really know the format of this weird file, but it looks like it might be sending -fstack-protector-strong to MSVC, which will be ignored:

we don't, the condition block if-else works like this, refs https://chromium.googlesource.com/external/gyp/+show/md-pages/docs/UserDocumentation.md#564

'conditions': [
      [<condition>, {
          true
      }, {
         false
     }]