node
node copied to clipboard
nodejs
src: remove UncheckedMalloc(0) workaround
Unlike UncheckedRealloc(p, 0), which returns a nullptr, both UncheckedMalloc(0) and UncheckedCalloc(0) currently perform fake allocations to return a non-nullptr.
Assuming that UncheckedMalloc(0) returns a non-nullptr is non-standard and we use other allocators as well (e.g., OPENSSL_malloc) that do not guarantee this behavior. It is the caller's responsibility to check that size != 0 implies UncheckedMalloc(size) != nullptr, and that's exactly what the checked variants (Malloc etc.) already do.
The current behavior is also inconsistent with UncheckedRealloc(), which always returns a nullptr when the size is 0, and with the documentation in src/README.md as well as with multiple comments in the source code.
https://github.com/nodejs/node/blob/0917626b96c2229ace91b55f72bc3faf152e74e9/src/README.md?plain=1#L954-L955
https://github.com/nodejs/node/blob/e62f6ce630c3ea9d9484d14bb892cb6836b250d4/src/util.h#L69-L78
https://github.com/nodejs/node/blob/e62f6ce630c3ea9d9484d14bb892cb6836b250d4/src/util-inl.h#L334-L340
This changes UncheckedMalloc(), UncheckedCalloc(), and UncheckedRealloc() to always return a nullptr when the size is 0 instead of doing fake allocations in UncheckedMalloc() and UncheckedCalloc() while returning a nullptr from UncheckedRealloc(). This is consistent with existing documentation and comments.
Refs: https://github.com/nodejs/node/issues/8571 Refs: https://github.com/nodejs/node/pull/8572
Not a fan. Around 2016 a lot of effort was spent tracking down bugs caused by different malloc() implementations. I don't relish the thought of revisiting that.
The current state, which might be the result of those efforts to "fix" the behavior of malloc(), is inconsistent though. On success, UncheckedMalloc(0) never returns a nullptr (and instead performs a useless allocation), but UncheckedRealloc(p, 0) does.
Even if we decide on some node-specific convention for Malloc() and friends, we still use other allocators such as OpenSSL_malloc(), which are not guaranteed to follow the same conventions.
In fact, there are very few call sites of Unchecked*(); most of Node.js uses the checked variants (Malloc(), Calloc(), Realloc()), automatic memory management (MallocedBuffer etc.), or OpenSSL allocators.
Semantically, it would probably be cleaner to return a std::optional<T*> to differentiate "allocation failed" from "a pointer to n allocated bytes", where the latter can be a nullptr. This would force call sites to check whether allocation succeeded. (On the other hand, the caller already knows the size of the allocation and should not care if the returned pointer is a nullptr if the size is 0.)
I am aware that OpenSSL sometimes behaves strangely when given a nullptr even when it should be valid, but IMHO Node.js itself should not make the same mistake.
I could change UncheckedCalloc() to always return a nullptr if MultiplyWithOverflowCheck(sizeof(T), n) == 0 so that the behavior of Unchecked*() will at least be consistent and not depend on the stdlib behavior, if that helps. But I do strongly feel that we should not make fake 1-byte allocations just to return a non-nullptr because the caller does not check the result correctly.
I'm sure you realize this but the reason n = 0 is turned into n = 1 is because node in many, many places diligently checks that UncheckedMalloc() succeeds (returns non-null) but not so diligently checks if n > 0.
I suppose you could change all instances of:
// ret = UncheckedMalloc(n);
CHECK_NOT_NULL(ret);
To:
CHECK_IMPLIES(n > 0, ret != nullptr);
But goshdarn, what a tedious pull request to write or review.
I'm sure you realize this but the reason
n = 0is turned inton = 1is because node in many, many places diligently checks thatUncheckedMalloc()succeeds (returns non-null) but not so diligently checks ifn > 0.
I do realize that, but I don't think that making fake allocations is the right approach to resolve the problem. I'd consider the condition you are describing a bug; it's non-standard and that assumption might fail if some other allocator is used (e.g, OpenSSL). In fact, it will already fail when node's own UncheckedRealloc() is used instead of/in addition to UncheckedMalloc(), and that makes no sense IMHO.
The current behavior is inconsistent (see UncheckedMalloc() versus UncheckedRealloc()) and also contradicts what multiple comments and src/README.md claim:
https://github.com/nodejs/node/blob/0917626b96c2229ace91b55f72bc3faf152e74e9/src/README.md?plain=1#L954-L955
https://github.com/nodejs/node/blob/e62f6ce630c3ea9d9484d14bb892cb6836b250d4/src/util.h#L69-L78
https://github.com/nodejs/node/blob/e62f6ce630c3ea9d9484d14bb892cb6836b250d4/src/util-inl.h#L334-L340
I suppose you could change all instances of:
// ret = UncheckedMalloc(n); CHECK_NOT_NULL(ret);To:
CHECK_IMPLIES(n > 0, ret != nullptr);
UncheckedMalloc() is never used with CHECK_NOT_NULL() because that's exactly what Malloc() is for, which does already use CHECK_IMPLIES().
The only cases worth looking at are those that use UncheckedMalloc() and then gracefully handle allocation failures, and there are very few of those.
As suggested in my previous comment, I have changed UncheckedCalloc() to also always return a nullptr when sizeof(T) * n == 0.
Unlike before, the behavior of UncheckedMalloc(), UncheckedCalloc(), and UncheckedRealloc() is now consistent in that all of them return nullptr when the requested size is 0 (as opposed to only UncheckedRealloc() returning a nullptr in that case while the other two perform fake allocations).
Also, unlike before, this matches internal documentation and comments in the code.