src: remove UncheckedMalloc(0) workaround

[tniessen]

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.

node/src/README.md

Lines 954 to 955 in 0917626

	The `UncheckedMalloc()`, `UncheckedRealloc()` and `UncheckedCalloc()` functions
	return `nullptr` in these cases (or when `size == 0`).

node/src/util.h

Lines 69 to 78 in e62f6ce

	// malloc(0) and realloc(ptr, 0) have implementation-defined behavior in
	// that the standard allows them to either return a unique pointer or a
	// nullptr for zero-sized allocation requests. Normalize by always using
	// a nullptr.
	template <typename T>
	inline T* UncheckedRealloc(T* pointer, size_t n);
	template <typename T>
	inline T* UncheckedMalloc(size_t n);
	template <typename T>
	inline T* UncheckedCalloc(size_t n);

node/src/util-inl.h

Lines 334 to 340 in e62f6ce

	// These should be used in our code as opposed to the native
	// versions as they abstract out some platform and or
	// compiler version specific functionality.
	// malloc(0) and realloc(ptr, 0) have implementation-defined behavior in
	// that the standard allows them to either return a unique pointer or a
	// nullptr for zero-sized allocation requests. Normalize by always using
	// a nullptr.

---

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: #8571
Refs: #8572

[nodejs-github-bot]

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

[bnoordhuis]

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.

[tniessen]

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.

[bnoordhuis]

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.

[tniessen]

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:

node/src/README.md

Lines 954 to 955 in 0917626

	The `UncheckedMalloc()`, `UncheckedRealloc()` and `UncheckedCalloc()` functions
	return `nullptr` in these cases (or when `size == 0`).

node/src/util.h

Lines 69 to 78 in e62f6ce

	// malloc(0) and realloc(ptr, 0) have implementation-defined behavior in
	// that the standard allows them to either return a unique pointer or a
	// nullptr for zero-sized allocation requests. Normalize by always using
	// a nullptr.
	template <typename T>
	inline T* UncheckedRealloc(T* pointer, size_t n);
	template <typename T>
	inline T* UncheckedMalloc(size_t n);
	template <typename T>
	inline T* UncheckedCalloc(size_t n);

node/src/util-inl.h

Lines 334 to 340 in e62f6ce

	// These should be used in our code as opposed to the native
	// versions as they abstract out some platform and or
	// compiler version specific functionality.
	// malloc(0) and realloc(ptr, 0) have implementation-defined behavior in
	// that the standard allows them to either return a unique pointer or a
	// nullptr for zero-sized allocation requests. Normalize by always using
	// a nullptr.

---

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.

[tniessen]

cc @nodejs/cpp-reviewers

[tniessen]

ping @nodejs/cpp-reviewers @bnoordhuis

[RafaelGSS]

I have looked at the patch and the PR description/discussion and both make sense to me. However, I haven't been around for so long to say if that will fall on some edge case. I'll leave my LGTM assuming the CI will be green.

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 1af6619