Node: napi_get_buffer_info(): change of behavior in v11.11.0

[jorangreef]

• Version: v11.11.0
• Platform: all platforms
• Subsystem: napi

A subtle change of behavior in napi_get_buffer_info() was introduced in v11.11.0.

For v11.10.0 and earlier, the following napi addon's assertions passed, even when the argv buffer value being retrieved was 0 bytes in length.

Since v11.11.0, however, the assert(*buffer != NULL); assertion below fails when the argv buffer value is 0 bytes in length.

This may not be a deal-breaker, and perhaps we need to update our assertions, but I first wanted to get to the bottom of why exactly this is happening, and why this changed in v11.11.0.

// converts a napi_value buffer argument to an unsigned char* buffer:
static int arg_buf(
  napi_env env,
  napi_value value,
  unsigned char** buffer,
  int* length,
  const char* error
) {
  assert(*buffer == NULL);
  assert(*length == 0);
  bool is_buffer;
  OK(napi_is_buffer(env, value, &is_buffer));
  if (!is_buffer) {
    napi_throw_error(env, NULL, error);
    return 0;
  }
  size_t size = 0;
  assert(napi_get_buffer_info(env, value, (void**) buffer, &size) == napi_ok);
  assert(*buffer != NULL);
  if (size > INT_MAX) {
    napi_throw_error(env, NULL, E_BUFFER_LENGTH);
    return 0;
  }
  *length = (int) size;
  assert(*length >= 0);
  return 1;
}

@addaleax, I know you did some recent work in https://github.com/nodejs/node/pull/26301, could that have anything to do with this?

[addaleax]

I know you did some recent work in #26301, could that have anything to do with this?

Yeah, that’s definitely possible.

This may not be a deal-breaker, and perhaps we need to update our assertions

I don’t think we can make guarantees about the contents of *buffer if size == 0 – you cannot dereference the pointer anyway, so it may hold any value.

but I first wanted to get to the bottom of why exactly this is happening, and why this changed in v11.11.0.

For that a full reproduction would be great, and in particular it’s good to know how you created the Buffer instance?

[jorangreef]

Thanks @addaleax

I don’t think we can make guarantees about the contents of *buffer if size == 0 – you cannot dereference the pointer anyway, so it may hold any value.

Sure, I don't mind updating the assertions. I would just like to understand the reason for the change. The assertions were helpful to ensure that certain logic had run, especially where some buffer arguments were required and others were optional. For example, in our async work complete handler, before deleting napi_value references, we would assert that the corresponding pointers were in fact defined.

For that a full reproduction would be great, and in particular it’s good to know how you created the Buffer instance?

Sure, here's a gist which reproduces exactly on v11.10.0 (passes) and v11.11.0 (fails):

https://gist.github.com/jorangreef/19956b854e7d5e245c8ecb9eb655f4cd

process.version=v11.10.0
testing Buffer.alloc(0)...
buffer_length=0
testing cipher.update(Buffer.alloc(0))...
buffer_length=0

process.version=v11.11.0
testing Buffer.alloc(0)...
buffer_length=0
testing cipher.update(Buffer.alloc(0))...
Assertion failed: (*buffer != NULL), function arg_buf, file ../binding.c, line 21.
Abort trap: 6

Here, if the addon prints buffer_length=0 it means that the buffer != NULL assertion passed. You can see in these results, that for v11.11.0, the assertion passes for buffers allocated using Buffer.alloc() but not for buffers allocated by cipher.update().

Really appreciate your help with this.

[addaleax]

I think this is a result of the added Resize() call in https://github.com/nodejs/node/commit/84e02b178ad14fae0df2a514e8a39bfa50ffdc2d#diff-801e3948990f4965a8ea4aca4a423864R4002 – before this, we returned a Buffer with the pointer that was originally allocated, but a size that may have been smaller than the original allocation, thus leaving the remaining memory around as unusable overhead. The added Resize() call may change the base pointer, and in the case of a length of 0, will return nullptr.

[jorangreef]

Thanks @addaleax

[jorangreef]

@addaleax,

Just out of interest, but perhaps this change of behavior will cause some ecosystem breakage?

For example, I updated our native addon's assertions to assert(buffer != NULL || size == 0).

And I would have thought this would have been enough.

However, our fuzz tests picked up strange behavior when instantiating an hmac using OpenSSL. Technically, an hmac can be instantiated with an empty key, however https://github.com/nodejs/node/commit/84e02b178ad14fae0df2a514e8a39bfa50ffdc2d#diff-801e3948990f4965a8ea4aca4a423864R4002 now means that it's possible for NULL to be passed to OpenSSL, which sees this and fails the initialization.

[jorangreef]

It looks like its turtles all the way down. 🐢

[jorangreef]

Instead of Resize() returning nullptr, it should probably rather statically allocate a guard page to return when length is 0, so that the return value looks like a valid pointer for programs that check (even if they never dereference).

[addaleax]

@jorangreef Okay, I’ve opened https://github.com/nodejs/node/pull/26731 to address this.

[jorangreef]

Thanks for fixing this quickly.