src: simplify memory management using `node::Malloc()` and friends#8482

addaleax · 2016-09-10T16:47:39Z

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/

addaleax · 2016-09-10T18:54:20Z

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 · 2016-09-10T21:32:53Z

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

bnoordhuis · 2016-09-11T08:16:40Z

src/cares_wrap.cc

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

bnoordhuis · 2016-09-11T08:38:00Z

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 · 2016-09-11T12:21:21Z

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

addaleax · 2016-09-11T14:54:54Z

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/

bnoordhuis · 2016-09-12T08:05:36Z

src/stream_wrap.cc

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

addaleax · 2016-09-12T09:09:58Z

New CI: https://ci.nodejs.org/job/node-test-commit/5013/

bnoordhuis · 2016-09-12T10:45:09Z

src/util-inl.h

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

bnoordhuis · 2016-09-12T10:48:27Z

LGTM

seishun · 2016-09-12T11:10:36Z

src/util-inl.h

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.

@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

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.

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

jasnell

LGTM

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]>

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]>

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]>

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]>

MylesBorins · 2016-10-06T23:44:40Z

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

Fishrock123 · 2016-10-10T16:42:40Z

depends on #7082

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]>

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]>

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]>

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]>

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]>

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 · 2017-10-29T12:17:09Z

v6.x: #16587

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

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]>

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]>

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]>

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]>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 10, 2016

addaleax added the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 10, 2016

addaleax force-pushed the allocation-rework branch 3 times, most recently from 9e3309c to f04c5edCompare September 10, 2016 18:03

bnoordhuis reviewed Sep 11, 2016
View reviewed changes

addaleax force-pushed the allocation-rework branch 2 times, most recently from 4b39181 to 1491d99Compare September 11, 2016 14:48

bnoordhuis reviewed Sep 12, 2016
View reviewed changes

src/stream_wrap.cc Outdated
Copy link
Member
bnoordhuisSep 12, 2016
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 force-pushed the allocation-rework branch from 1491d99 to f46329cCompare September 12, 2016 09:07

bnoordhuis reviewed Sep 12, 2016
View reviewed changes

seishun reviewed Sep 12, 2016
View reviewed changes

addaleax force-pushed the allocation-rework branch from f46329c to d03ff03Compare September 13, 2016 08:49

jasnell approved these changes Sep 15, 2016
View reviewed changes

addaleax added the blocked PRs that are blocked by other issues or PRs. label Sep 17, 2016

addaleax mentioned this pull request Sep 17, 2016
src: Malloc/Calloc size 0 returns non-null pointer #8572
Closed
3 tasks

addaleax deleted the allocation-rework branch September 30, 2016 15:12

MylesBorins added the dont-land-on-v4.x label Oct 6, 2016

addaleax mentioned this pull request Oct 7, 2016
core: normalize malloc, realloc #7564
Closed
2 tasks

Fishrock123 added the dont-land-on-v6.x label Oct 10, 2016

gibfahn mentioned this pull request Jun 15, 2017
Auditing for 6.11.1 nodejs/Release#230
Closed
3 tasks

XadillaX mentioned this pull request Jun 21, 2017
(v6.x backport) dns: fix crash while using dns.setServers after dns.resolve4 #13724
Closed
3 tasks

bnoordhuis mentioned this pull request Oct 29, 2017
[v6.x backport] src: simplify memory management using node::Malloc() and friends #16587
Closed

bnoordhuis removed the dont-land-on-v6.x label Oct 29, 2017

MylesBorins added the backported-to-v6.x label Nov 14, 2017

MylesBorins mentioned this pull request Nov 21, 2017
v6.12.1 proposal #17180
Merged

Uh oh!

src: simplify memory management using node::Malloc() and friends#8482

src: simplify memory management using node::Malloc() and friends #8482

Uh oh!

Conversation

addaleax commented Sep 10, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

addaleax commented Sep 10, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Sep 10, 2016

Uh oh!

bnoordhuisSep 11, 2016

Choose a reason for hiding this comment

Uh oh!

addaleaxSep 11, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Sep 11, 2016

Uh oh!

seishun commented Sep 11, 2016

Uh oh!

addaleax commented Sep 11, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuisSep 12, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 12, 2016

Uh oh!

bnoordhuisSep 12, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Sep 12, 2016

Uh oh!

seishunSep 12, 2016

Choose a reason for hiding this comment

Uh oh!

addaleaxSep 12, 2016

Choose a reason for hiding this comment

Uh oh!

seishunSep 13, 2016

Choose a reason for hiding this comment

Uh oh!

addaleaxSep 13, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Oct 6, 2016

Uh oh!

Fishrock123 commented Oct 10, 2016

Uh oh!

bnoordhuis commented Oct 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

src: simplify memory management using `node::Malloc()` and friends#8482

src: simplify memory management using `node::Malloc()` and friends #8482

addaleax commented Sep 10, 2016•
edited
Loading

addaleax commented Sep 10, 2016•
edited
Loading

addaleax commented Sep 11, 2016•
edited
Loading

bnoordhuisSep 12, 2016•
edited
Loading