Repository navigation
napi_get_buffer_info(): change of behavior in v11.11.0 #26514
Description
Activity
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
*bufferifsize == 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?
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=0process.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: 6Here, if the addon prints
buffer_length=0it means that thebuffer != NULLassertion passed. You can see in these results, that for v11.11.0, the assertion passes for buffers allocated usingBuffer.alloc()but not for buffers allocated bycipher.update().Really appreciate your help with this.
I think this is a result of the added
Resize()call in 84e02b1#diff-801e3948990f4965a8ea4aca4a423864R4002 – before this, we returned aBufferwith 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 addedResize()call may change the base pointer, and in the case of a length of0, will returnnullptr.Reacted by Joran Dirk GreefThanks @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 84e02b1#diff-801e3948990f4965a8ea4aca4a423864R4002 now means that it's possible for NULL to be passed to OpenSSL, which sees this and fails the initialization.
It looks like its turtles all the way down. 🐢
Instead of
Resize()returningnullptr, 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).Reacted by Anna Henningsen- added a commit that references this issue
on Mar 18, 2019 @jorangreef Okay, I’ve opened #26731 to address this.
Reacted by Joran Dirk Greef- added a commit that references this issue
on Mar 21, 2019 Thanks for fixing this quickly.
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.
@addaleax, I know you did some recent work in #26301, could that have anything to do with this?