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: simplify memory management using node::Malloc() and friends #8482

Merged
merged 4 commits into from
Sep 29, 2016

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Sep 10, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

Description of change
  • Add an optional size param to node::Malloc() and node::Realloc() in calloc(3)-style and use proper overflow detection.
  • Make the allocation methods templates so that they directly give the correct return type, removing a number of static_casts to the desired pointer types.
  • Add shortcuts for the node::Malloc() + CHECK_NE(·, nullptr) combinations that often occur together in the codebase.
  • Call v8::Isolate::GetCurrent()->LowMemoryNotification() when an allocation fails to give V8 a chance to clean up and return memory before retrying (and possibly giving up).

Any of the changes here can be left out but I believe that they all pretty much make sense to have.

CI: https://ci.nodejs.org/job/node-test-commit/4994/
CI: https://ci.nodejs.org/job/node-test-commit/4995/
CI: https://ci.nodejs.org/job/node-test-commit/4996/
CI: https://ci.nodejs.org/job/node-test-commit/4997/
CI: https://ci.nodejs.org/job/node-test-commit/5008/
CI: https://ci.nodejs.org/job/node-test-commit/5013/
CI: https://ci.nodejs.org/job/node-test-commit/5025/

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 10, 2016
@addaleax addaleax added the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 10, 2016
@addaleax addaleax force-pushed the allocation-rework branch 3 times, most recently from 9e3309c to f04c5ed Compare September 10, 2016 18:03
@addaleax
Copy link
Member Author

addaleax commented Sep 10, 2016

Woooo, internal compiler error on Windows: https://ci.nodejs.org/job/node-compile-windows/4115/label=win-vs2015/console

Edit for future reference: This happened with HEAD = f04c5ed4bc1ae3dcb639da469605ae1ea47ca612

One can probably eliminate that by playing with the code a bit, but maybe this is interesting for someone from @nodejs/platform-windows ?

@addaleax
Copy link
Member Author

Also – @joshgav do you want to/should you be added to the platform-windows team?

@@ -142,8 +142,7 @@ static void ares_poll_close_cb(uv_handle_t* watcher) {

/* Allocates and returns a new node_ares_task */
static node_ares_task* ares_task_create(Environment* env, ares_socket_t sock) {
node_ares_task* task =
static_cast<node_ares_task*>(node::Malloc(sizeof(*task)));
node_ares_task* task = node::Malloc<node_ares_task>(1);
Copy link
Member

Choose a reason for hiding this comment

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

You're welcome to write this as auto task = ... while you're here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@bnoordhuis
Copy link
Member

Woooo, internal compiler error on Windows: https://ci.nodejs.org/job/node-compile-windows/4115/label=win-vs2015/console

Interesting. For all its faults I've never gotten VS to ICE.

@seishun
Copy link
Contributor

seishun commented Sep 11, 2016

I'm not getting an ICE locally. Maybe try updating VS on CI?

@addaleax addaleax force-pushed the allocation-rework branch 2 times, most recently from 4b39181 to 1491d99 Compare September 11, 2016 14:48
@addaleax
Copy link
Member Author

addaleax commented Sep 11, 2016

Trying to run a new CI at https://ci.nodejs.org/job/node-test-pull-request/4012/ but it seems to be stuck reading data from Github?

Edit: https://ci.nodejs.org/job/node-test-commit/5008/

@@ -204,7 +198,7 @@ void StreamWrap::OnReadImpl(ssize_t nread,
return;
}

char* base = static_cast<char*>(node::Realloc(buf->base, nread));
char* base = node::UncheckedRealloc(buf->base, nread);
Copy link
Member

Choose a reason for hiding this comment

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

I'd make this a checked realloc. It can't really fail because it shrinks but I doubt the extra check hurts.

@addaleax
Copy link
Member Author

T MultiplyWithOverflowCheck(T a, T b) {
T ret = a * b;
if (a != 0)
CHECK_EQ(b, ret / a);
Copy link
Member

@bnoordhuis bnoordhuis Sep 12, 2016

Choose a reason for hiding this comment

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

EDIT: No, I got that wrong. The branch is always taken because a == sizeof(T) and therefore never zero.

@bnoordhuis
Copy link
Member

LGTM

@@ -229,30 +229,74 @@ bool StringEqualNoCaseN(const char* a, const char* b, size_t length) {
return true;
}

template <typename T>
T MultiplyWithOverflowCheck(T a, T b) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think making this a template is a good idea. It can't be used with signed types since signed overflow is UB, so the check could be optimized out.

Copy link
Member Author

Choose a reason for hiding this comment

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

@seishun How strongly do you feel about this? I don’t really care but it might be nice to have a generic utility if one is ever needed again. I can add a static_assert to check for unsignedness, if you want.

Also, I’m surprised, but your worries are not only theoretical; clang actually would optimize the check away for signed integers… oO

Copy link
Contributor

Choose a reason for hiding this comment

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

I can add a static_assert to check for unsignedness, if you want.

That works, but I would personally prefer to have a function that works just with size_t. That way you can see that it doesn't work with signed values from the signature. Your call, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

@seishun ¯_(ツ)_/¯ updated with size_t :)

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM

@addaleax addaleax added the blocked PRs that are blocked by other issues or PRs. label Sep 17, 2016
jasnell pushed a commit that referenced this pull request Sep 29, 2016
Adds an optional second parameter to `node::Malloc()` and
an optional third parameter to `node::Realloc()` giving the
size/number of items to be allocated, in the style of `calloc(3)`.

Use a proper overflow check using division;
the previous `CHECK_GE(n * size, n);` would not detect all cases
of overflow (e.g. `size == SIZE_MAX / 2 && n == 3`).

PR-URL: #8482
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
jasnell pushed a commit that referenced this pull request Sep 29, 2016
Pass the desired return type directly to the allocation functions,
so that the resulting `static_cast` from `void*` becomes unneccessary
and the return type can be use as a reasonable default value for the
`size` parameter.

PR-URL: #8482
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
jasnell pushed a commit that referenced this pull request Sep 29, 2016
Provide shortcut `node::CheckedMalloc()` and friends that
replace `node::Malloc()` + `CHECK_NE(·, nullptr);` combinations
in a few places.

PR-URL: #8482
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
jasnell pushed a commit that referenced this pull request Sep 29, 2016
Call `v8::Isolate::GetCurrent()->LowMemoryNotification()` when
an allocation fails to give V8 a chance to clean up and return
memory before retrying (and possibly giving up).

PR-URL: #8482
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
@addaleax addaleax deleted the allocation-rework branch September 30, 2016 15:12
@MylesBorins
Copy link
Contributor

@addaleax I've set this as do not land... please feel free to change if it should be backported

@Fishrock123
Copy link
Contributor

depends on #7082

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Nov 18, 2016
Make cctest valgrind-clean again by freeing heap-allocated memory.
Overlooked in commit ea94086 ("src: provide allocation + nullptr check
shortcuts.")

PR-URL: nodejs#9667
Refs: nodejs#8482
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
addaleax pushed a commit that referenced this pull request Nov 22, 2016
Make cctest valgrind-clean again by freeing heap-allocated memory.
Overlooked in commit ea94086 ("src: provide allocation + nullptr check
shortcuts.")

PR-URL: #9667
Refs: #8482
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
bnoordhuis pushed a commit to bnoordhuis/io.js that referenced this pull request Oct 29, 2017
Adds an optional second parameter to `node::Malloc()` and
an optional third parameter to `node::Realloc()` giving the
size/number of items to be allocated, in the style of `calloc(3)`.

Use a proper overflow check using division;
the previous `CHECK_GE(n * size, n);` would not detect all cases
of overflow (e.g. `size == SIZE_MAX / 2 && n == 3`).

PR-URL: nodejs#8482
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
bnoordhuis pushed a commit to bnoordhuis/io.js that referenced this pull request Oct 29, 2017
Pass the desired return type directly to the allocation functions,
so that the resulting `static_cast` from `void*` becomes unneccessary
and the return type can be use as a reasonable default value for the
`size` parameter.

PR-URL: nodejs#8482
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
bnoordhuis pushed a commit to bnoordhuis/io.js that referenced this pull request Oct 29, 2017
Provide shortcut `node::CheckedMalloc()` and friends that
replace `node::Malloc()` + `CHECK_NE(·, nullptr);` combinations
in a few places.

PR-URL: nodejs#8482
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
bnoordhuis pushed a commit to bnoordhuis/io.js that referenced this pull request Oct 29, 2017
Call `v8::Isolate::GetCurrent()->LowMemoryNotification()` when
an allocation fails to give V8 a chance to clean up and return
memory before retrying (and possibly giving up).

PR-URL: nodejs#8482
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
@bnoordhuis
Copy link
Member

v6.x: #16587

Removed the dont-land-on-v6.x label.

MylesBorins pushed a commit that referenced this pull request Nov 14, 2017
Adds an optional second parameter to `node::Malloc()` and
an optional third parameter to `node::Realloc()` giving the
size/number of items to be allocated, in the style of `calloc(3)`.

Use a proper overflow check using division;
the previous `CHECK_GE(n * size, n);` would not detect all cases
of overflow (e.g. `size == SIZE_MAX / 2 && n == 3`).

Backport-PR-URL: #16587
PR-URL: #8482
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 14, 2017
Pass the desired return type directly to the allocation functions,
so that the resulting `static_cast` from `void*` becomes unneccessary
and the return type can be use as a reasonable default value for the
`size` parameter.

Backport-PR-URL: #16587
PR-URL: #8482
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 14, 2017
Provide shortcut `node::CheckedMalloc()` and friends that
replace `node::Malloc()` + `CHECK_NE(·, nullptr);` combinations
in a few places.

Backport-PR-URL: #16587
PR-URL: #8482
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 14, 2017
Call `v8::Isolate::GetCurrent()->LowMemoryNotification()` when
an allocation fails to give V8 a chance to clean up and return
memory before retrying (and possibly giving up).

Backport-PR-URL: #16587
PR-URL: #8482
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants