CI: https://ci.nodejs.org/job/node-test-pull-request/46453/

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 = 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 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:

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.

cc @nodejs/cpp-reviewers

ping @nodejs/cpp-reviewers @bnoordhuis

CI: https://ci.nodejs.org/job/node-test-pull-request/47090/

Landed in 1af6619