Node: Segfault on v12.19.0

[jambagigirish]

• Version:v12.19.0
• Platform: RHEL
• Subsystem:

What steps will reproduce the bug?

Using node js and running the component test with cucumber-js

How often does it reproduce? Is there a required condition?

After Upgrading to v12.19.0 - issue has been consistent

What is the expected behavior?

What do you see instead?

/home/user1/GIT/cp1/r1/r1/node_modules/segfault-handler/build/Release/segfault-handler.node(+0x2c61)[0x7f9e881fbc61]
/lib64/libpthread.so.0(+0xf6d0)[0x7f9ea0eb06d0]
node[0x9cf96e]
node(_ZN2v88internal13GlobalHandles40InvokeSecondPassPhantomCallbacksFromTaskEv+0x19b)[0xd17c4b]
node(_ZN2v88internal14CancelableTask3RunEv+0x23)[0xc953e3]
node(_ZN4node22PerIsolatePlatformData17RunForegroundTaskESt10unique_ptrIN2v84TaskESt14default_deleteIS3_EE+0xc4)[0xa86d54]
node(_ZN4node22PerIsolatePlatformData28FlushForegroundTasksInternalEv+0x165)[0xa87a15]
node[0x136f0ae]
node[0x1382165]
node(uv_run+0x11f)[0x136f8ef]
node(_ZN4node16NodeMainInstance3RunEv+0x1f6)[0xa5aac6]
node(_ZN4node5StartEiPPc+0x2ac)[0x9e85cc]
/lib64/libc.so.6(__libc_start_main+0xf5)[0x7f9ea0af6445]
node[0x9819b5]

Additional information

I am using native code build with node-gyp

[addaleax]

I am using native code build with node-gyp

Can you share that native code?

[jambagigirish]

this is one of the cpp files where nodejs sends data to convert/map to c++ byte array to send to sockets

static void Activate(const FunctionCallbackInfo<Value>& args);

///// CODE
void ActivationInit(v8::Local<v8::Object> &exports, HMODULE handle1)
{
    NODE_SET_METHOD(exports, "Activate", Activate);
}

Isolate* isolate = args.GetIsolate();
  unsigned char szUser[256];


  if (args.Length() != 4)
  {
    HandleWrongParams(isolate);
    return;
  }
  try
  {
    unsigned long param1 = getParam1(args[0], isolate);
    unsigned long param2 = getParam2(args[1], isolate);
    unsigned long param3 = getParam3(args[2], isolate);
    memset(szUser, 0, 256);
    getParamUString(args[3], szUser, 256, isolate);

    BOOL result= SendActivateMessageToSocket(param1, param2, param4, szUser);
    Local<Boolean> res = Boolean::New(isolate, result);
    args.GetReturnValue().Set(res);
  }
  catch(exception &e)
  {
    if (NULL != isolate)
    {
        isolate->ThrowException(Exception::TypeError(String::NewFromUtf8(isolate, e.what())));
    }
    return;
  }

DWORD getParam1(Local<Value> val, Isolate* isolate)
{
  if (val->NumberValue( isolate->GetCurrentContext()).ToChecked() < 0) throw invalid_argument("Negative value received.");
  return (DWORD)(val->Uint32Value(isolate->GetCurrentContext()).ToChecked());
}

Update: node js calls Activate() from nodejs code

[addaleax]

this is one of the cpp files where nodejs sends data to convert/map to c++ byte array to send to sockets

This is obviously an incomplete file.

[jambagigirish]

cpp files are too big to share here. with version 12.18.00 I didn't see any issues. Are there any breaking changes from version 12.18 to 12.19?

[addaleax]

Are there any breaking changes from version 12.18 to 12.19?

None that are intentionally breaking, no, and none that were reported so far.

cpp files are too big to share here

Okay, well, let me be clear here: I’m trying to help you. If you can’t show us a reproduction of the issue, that’s harder. Without looking at the causing source code, that’s even harder. The file size limit for Github comment attachements is 25 MB, and I honestly doubt a bit that you have C++ files larger than that (the biggest .cc file in all of Node.js is less than 1 MB, for reference).

Looking at your code to try to figure out what’s going on is a offer from our side that I don’t really need to make here – I do this in my free time these days, and you’re using Node.js and all the work put into it for free as well. If you can’t share your source code and you’re doing closed-source development, that means you’re taking on responsibility for solving issues with your source code yourself. That’s fine – you’re absolutely free to do that. But if you want help, you need to provide information so that people can actually help you.

That being said, the stack trace points to an issue with garbage collection. That also means that the transition from 12.18.0 to 12.19.0 might not even be the main cause here, and that GC timing has changed instead. If you do suspect that this might be a bug in Node.js, and you cannot share more information, you can bisect the range between the two tags on github to figure out what commit introduced this crash.

[jambagigirish]

Thanks for you response. Main issue is that native code is divided over multiple files as shown below. all these files do the same processing as shown in above snippet. As its a production/copyrighted c++ code I can't share the much c++ code here. Sorry for that.

I am using node-gyp library to build c++ code. May be I can divide the issue between 2 repos.

After trying to build debug version I wasn't able to get the valid-readable stack trace.

I tried to generate something similar to this but no luck-https://github.com/nodejs/node/issues/33283

[ronag]

@jambagigirish I think your best bet is to build and bisect Node yourself and try to find which commit caused the regression you are seeing.

Alternatively, try to create a minimal reproducible example that we can use, otherwise there is not much we can do to help.

[koonpeng]

I am getting the same error (similar backtrace) as well with rclnodejs. There is a reproducible code snippet posted here https://github.com/RobotWebTools/rclnodejs/issues/713, however running it requires ros2.

[koonpeng]

This appears to be the commit causing the error https://github.com/nodejs/node/commit/a6b655614f03e073b9c60f3d71ed884c5af32ffc. I'm guessing some destructors is failing to check if the finalize callback is valid.

[hertzg]

I have also noticed a segfault in my package which is using ffi-napi. There were no code changes on the package side but the segfault only happens on 12.19.0.

| Node version | Release Date | Workflow Run |
| ------------ | ------------ | --------------------------------------------------------------------------------------------- |
| v10.22.1 | 2020-09-15 | ✔️ Passed |
| v12.18.4 | 2020-09-15 | ✔️ Passed |
| v14.11.0 | 2020-09-15 | ✔️ Passed |
| v14.12.0 | 2020-09-22 | ✔️ Passed |
| v14.13.0 | 2020-09-29 | ✔️ Passed |
| v12.19.0 | 2020-10-06 | 🛑 Failed |
| v14.13.1 | 2020-10-07 | ✔️ Passed |
| v14.14.0 | 2020-10-15 | ✔️ Passed |

[Flarna]

fyi @nodejs/n-api

[mhdawson]

created https://github.com/nodejs/abi-stable-node/issues/411 to make sure we talk about it in the next N-API team meeting.

[mhdawson]

From an initial look at https://github.com/nodejs/node/commit/a6b655614f03e073b9c60f3d71ed884c5af32ffc, I don't think it's correct because Finalize (https://github.com/nodejs/node/blob/a6b655614f03e073b9c60f3d71ed884c5af32ffc/src/js_native_api_v8.cc#L270) can still be called even if the user did not provide a finalizer and _finalize_callback is NULL.

If that's correct we should likely back out the change.

[mhdawson]

We discussed in the N-API meeting today and @gabrielschulhof agreed with my assessment, will create PR to revert

[mhdawson]

PR for revert: https://github.com/nodejs/node/pull/35777

[hertzg]

Will there be a patch release (having the revert commit) for v12 Or only after a proper patch is introduced?

[mhdawson]

@hertzg https://github.com/nodejs/node/pull/35777 is tagged for backport to 12.x and 14.x (based on the lts-watch-v12.x and lts-watch-v14.x) labels. Changes typically need to "bake" a bit in master/current before going back to LTS releases, but as a revert I'm thinking this might be able to skip that. @BethGriggs what's your thoughts on that ?

From https://github.com/nodejs/Release/issues/494 , It looks like there might be a patch release but it does not seem to have a date and then the next planned release for the 12.x line is v12.20.0 and is scheduled for 17 Nov, it would be nice to get it into that one at the latest.

[BethGriggs]

@mhdawson just to close the loop, @MylesBorins has pulled https://github.com/nodejs/node/pull/35777 into the v12.20.0 release proposal (https://github.com/nodejs/node/pull/35950)