Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: remove UncheckedMalloc(0) workaround #44543

Merged

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Sep 6, 2022

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 nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Sep 6, 2022
@tniessen tniessen force-pushed the src-remove-malloc-0-workaround branch from 9d3691f to 7d5bd1d Compare September 6, 2022 17:43
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 6, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 6, 2022
@nodejs-github-bot

This comment was marked as outdated.

@tniessen tniessen force-pushed the src-remove-malloc-0-workaround branch from 7d5bd1d to 29bc634 Compare September 6, 2022 19:51
@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 6, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 6, 2022
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

tniessen commented Sep 6, 2022

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
Copy link
Member

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.

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.

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: nodejs#8571
Refs: nodejs#8572
@tniessen tniessen force-pushed the src-remove-malloc-0-workaround branch from 29bc634 to 8dae03b Compare September 8, 2022 10:15
@tniessen
Copy link
Member Author

tniessen commented Sep 8, 2022

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
Copy link
Member Author

cc @nodejs/cpp-reviewers

@tniessen tniessen added the review wanted PRs that need reviews. label Sep 17, 2022
@tniessen
Copy link
Member Author

tniessen commented Oct 1, 2022

ping @nodejs/cpp-reviewers @bnoordhuis

Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 1, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 1, 2022
@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 1, 2022
@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 2, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 2, 2022
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 5, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 5, 2022
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@tniessen tniessen added commit-queue Add this label to land a pull request using GitHub Actions. and removed review wanted PRs that need reviews. labels Oct 5, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 5, 2022
@nodejs-github-bot nodejs-github-bot merged commit 1af6619 into nodejs:main Oct 5, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 1af6619

danielleadams pushed a commit that referenced this pull request Oct 11, 2022
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.

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
PR-URL: #44543
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants