src: simplify memory management using `node::Malloc()` and friends by addaleax · Pull Request #8482 · nodejs/node

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>

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 <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

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 <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>

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 f04c5ed Compare 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 1491d99 Compare September 11, 2016 14:48

bnoordhuis reviewed Sep 12, 2016
View reviewed changes

addaleax force-pushed the allocation-rework branch from 1491d99 to f46329c Compare 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 d03ff03 Compare 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!

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!

bnoordhuis Sep 11, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax Sep 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!

bnoordhuis Sep 12, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 12, 2016

Uh oh!

bnoordhuis Sep 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!

seishun Sep 12, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax Sep 12, 2016

Choose a reason for hiding this comment

Uh oh!

seishun Sep 13, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax Sep 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

addaleax commented Sep 10, 2016 •

edited

Loading

addaleax commented Sep 10, 2016 •

edited

Loading

addaleax commented Sep 11, 2016 •

edited

Loading

bnoordhuis Sep 12, 2016 •

edited

Loading