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

core: normalize malloc, realloc #7564

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@mhdawson
Member

mhdawson commented Jul 6, 2016

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

core

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.

- Introduce node::malloc, node::realloc and node::calloc that should
  be used throught our source.
- Update all existing node source files to use the new functions
  instead of the native allocation functions.
@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Jul 6, 2016

Member

FYI those involved in the original discussion @bnoordhuis, @jasnell, @ChALkeR

Member

mhdawson commented Jul 6, 2016

FYI those involved in the original discussion @bnoordhuis, @jasnell, @ChALkeR

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Jul 6, 2016

Member

Bleh, I hate this let's-add-more-macros approach. If you want to eradicate malloc(0), move everything over to new / new (std::nothrow). That avoids platform-specific macro magic and it cleans up our inconsistent memory management.

Member

bnoordhuis commented Jul 6, 2016

Bleh, I hate this let's-add-more-macros approach. If you want to eradicate malloc(0), move everything over to new / new (std::nothrow). That avoids platform-specific macro magic and it cleans up our inconsistent memory management.

@mscdex mscdex added lib / src and removed build inspector labels Jul 6, 2016

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Jul 6, 2016

Member

LGTM with green CI but would prefer @bnoordhuis to be happy with it also.

Member

jasnell commented Jul 6, 2016

LGTM with green CI but would prefer @bnoordhuis to be happy with it also.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Jul 7, 2016

Member

I'll work on moving everything over to new[] / delete[].

Member

bnoordhuis commented Jul 7, 2016

I'll work on moving everything over to new[] / delete[].

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Jul 8, 2016

Member

@bnoordhuis is that something you are working now ? Just want to get a feel for whether its the short term fix or not.

Member

mhdawson commented Jul 8, 2016

@bnoordhuis is that something you are working now ? Just want to get a feel for whether its the short term fix or not.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Jul 8, 2016

Member

Yes.

Member

bnoordhuis commented Jul 8, 2016

Yes.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Aug 2, 2016

Member

Just wondering how this is going, want to be sure we have a fix in before LTS is cut for 6.x (still a ways off I know).

Member

mhdawson commented Aug 2, 2016

Just wondering how this is going, want to be sure we have a fix in before LTS is cut for 6.x (still a ways off I know).

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Aug 3, 2016

Member

I did some work on it already (and most of that has landed) but fully moving over to new/delete isn't possible short term.

In some places, notably the node::Buffer API, we leak the implementation detail that memory is managed with malloc/free. Since mixing C-style and C++-style dynamic memory is undefined behavior, we're stuck with that for the foreseeable future.

Member

bnoordhuis commented Aug 3, 2016

I did some work on it already (and most of that has landed) but fully moving over to new/delete isn't possible short term.

In some places, notably the node::Buffer API, we leak the implementation detail that memory is managed with malloc/free. Since mixing C-style and C++-style dynamic memory is undefined behavior, we're stuck with that for the foreseeable future.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Aug 3, 2016

Member

Ok so does that mean we sill need to land this PR to resolve the current issues on AIX ?

Member

mhdawson commented Aug 3, 2016

Ok so does that mean we sill need to land this PR to resolve the current issues on AIX ?

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Aug 4, 2016

Member

@bnoordhuis you ok with this change given that the move away from malloc/free is a longer term effort or do you have some alternative suggestion ?

Member

mhdawson commented Aug 4, 2016

@bnoordhuis you ok with this change given that the move away from malloc/free is a longer term effort or do you have some alternative suggestion ?

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Aug 5, 2016

Member

I want to take a slightly different approach. I have some in-progress work that I can PR next week.

Member

bnoordhuis commented Aug 5, 2016

I want to take a slightly different approach. I have some in-progress work that I can PR next week.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Aug 5, 2016

Member

Ok, thanks.

Member

mhdawson commented Aug 5, 2016

Ok, thanks.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Aug 11, 2016

Member

Any update ?

Member

mhdawson commented Aug 11, 2016

Any update ?

@mhdawson mhdawson referenced this pull request Aug 11, 2016

Closed

test: mark test failing on AIX as flaky #8065

2 of 2 tasks complete

mhdawson added a commit to mhdawson/io.js that referenced this pull request Aug 11, 2016

test: mark test failing on AIX as flaky
We now have adequate AIX hardware to add AIX to
the regular regression runs.

However, there are a couple of failing tests even
though AIX was green at one point.  This PR
marks those tests as flaky so that we can add AIX
so that we can spot any new regressions without making
the builds RED

The tests are being worked under the following PRs

- being worked under nodejs#7564
test-async-wrap-post-did-throw
test-async-wrap-throw-from-callback
test-crypto-random

- being worked under nodejs#7973
test-stdio-closed

- covered by nodejs#3796
test-debug-signal-cluster

mhdawson added a commit that referenced this pull request Aug 11, 2016

test: mark test failing on AIX as flaky
We now have adequate AIX hardware to add AIX to
the regular regression runs.

However, there are a couple of failing tests even
though AIX was green at one point.  This PR
marks those tests as flaky so that we can add AIX
so that we can spot any new regressions without making
the builds RED

The tests are being worked under the following PRs

- being worked under #7564
test-async-wrap-post-did-throw
test-async-wrap-throw-from-callback
test-crypto-random

- being worked under #7973
test-stdio-closed

- covered by #3796
test-debug-signal-cluster

PR-URL: #8065
Reviewed-By: joaocgreis - João Reis <reis@janeasystems.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

cjihrig added a commit that referenced this pull request Aug 11, 2016

test: mark test failing on AIX as flaky
We now have adequate AIX hardware to add AIX to
the regular regression runs.

However, there are a couple of failing tests even
though AIX was green at one point.  This PR
marks those tests as flaky so that we can add AIX
so that we can spot any new regressions without making
the builds RED

The tests are being worked under the following PRs

- being worked under #7564
test-async-wrap-post-did-throw
test-async-wrap-throw-from-callback
test-crypto-random

- being worked under #7973
test-stdio-closed

- covered by #3796
test-debug-signal-cluster

PR-URL: #8065
Reviewed-By: joaocgreis - João Reis <reis@janeasystems.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Aug 22, 2016

Member

@bnoordhuis how close are you to a fix. At this point I think we either need it this week or we land the original PR and then revert once you have a better fix.

Member

mhdawson commented Aug 22, 2016

@bnoordhuis how close are you to a fix. At this point I think we either need it this week or we land the original PR and then revert once you have a better fix.

Trott added a commit to Trott/io.js that referenced this pull request Aug 31, 2016

crypto: make malloc failure check cross-platform
`malloc(0)` may return NULL on some platforms. Do not report
out-of-memory error unless `malloc` was passed a number greater than
`0`.

Fixes: nodejs#7564
PR-URL: nodejs#8352

@bnoordhuis bnoordhuis referenced this pull request Aug 31, 2016

Closed

crypto: make malloc failure check cross-platform #8352

2 of 2 tasks complete
@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Sep 1, 2016

Member

This needs a rebase and I want to suggest an alternative to the #ifdef logic in util.h. Can you provide wrappers around calloc/malloc/realloc that take care of platform differences? Basically this:

void* Realloc(void* pointer, size_t size) {
  if (size == 0) {
    free(pointer);
    return nullptr;
  }
  return realloc(pointer, size);
}

void* Malloc(size_t size) {
  return Realloc(nullptr, size);
}

void* Calloc(size_t n, size_t size) {
  CHECK_GT(size, 0);
  CHECK_GE(n * size, n);  // Overflow guard.
  if (n == 0) return nullptr;
  return calloc(n, size);
}
Member

bnoordhuis commented Sep 1, 2016

This needs a rebase and I want to suggest an alternative to the #ifdef logic in util.h. Can you provide wrappers around calloc/malloc/realloc that take care of platform differences? Basically this:

void* Realloc(void* pointer, size_t size) {
  if (size == 0) {
    free(pointer);
    return nullptr;
  }
  return realloc(pointer, size);
}

void* Malloc(size_t size) {
  return Realloc(nullptr, size);
}

void* Calloc(size_t n, size_t size) {
  CHECK_GT(size, 0);
  CHECK_GE(n * size, n);  // Overflow guard.
  if (n == 0) return nullptr;
  return calloc(n, size);
}

mhdawson and others added some commits Sep 1, 2016

core: normalize malloc, realloc
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.

- Introduce node::malloc, node::realloc and node::calloc that should
  be used throught our source.
- Update all existing node source files to use the new functions
  instead of the native allocation functions.
crypto: make malloc failure check cross-platform
`malloc(0)` may return NULL on some platforms. Do not report
out-of-memory error unless `malloc` was passed a number greater than
`0`.

Fixes: #7564
PR-URL: #8352
@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Sep 1, 2016

Member

@bnoordhuis rebased and updated to use functions instead of macros as you suggested.

CI run here: https://ci.nodejs.org/job/node-test-pull-request/3922/

Member

mhdawson commented Sep 1, 2016

@bnoordhuis rebased and updated to use functions instead of macros as you suggested.

CI run here: https://ci.nodejs.org/job/node-test-pull-request/3922/

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Sep 2, 2016

Member

Failure on arm is infra problem that pre-existed
Failure on OSX looks unrelated but will launch another build just in case

not ok 606 parallel/test-fs-watch-recursive
# TIMEOUT

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

Member

mhdawson commented Sep 2, 2016

Failure on arm is infra problem that pre-existed
Failure on OSX looks unrelated but will launch another build just in case

not ok 606 parallel/test-fs-watch-recursive
# TIMEOUT

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

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Sep 2, 2016

Member

CI run looks good except for arm fanned job that was also failing in previous runs.

Member

mhdawson commented Sep 2, 2016

CI run looks good except for arm fanned job that was also failing in previous runs.

@mhdawson mhdawson changed the title from src: temporary malloc fix for AIX to core: normalize malloc, realloc Sep 2, 2016

@@ -180,7 +180,8 @@ class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator {
inline uint32_t* zero_fill_field() { return &zero_fill_field_; }
virtual void* Allocate(size_t size); // Defined in src/node.cc
virtual void* AllocateUninitialized(size_t size) { return malloc(size); }
virtual void* AllocateUninitialized(size_t size)
{ return node::Malloc(size); }

This comment has been minimized.

@bnoordhuis

bnoordhuis Sep 2, 2016

Member

The linter accepts this?

@bnoordhuis

bnoordhuis Sep 2, 2016

Member

The linter accepts this?

This comment has been minimized.

@mhdawson

mhdawson Sep 2, 2016

Member

It complained when it was on one line and passed with it this way !

@mhdawson

mhdawson Sep 2, 2016

Member

It complained when it was on one line and passed with it this way !

@@ -54,7 +54,7 @@ class ExternString: public ResourceType {
return scope.Escape(String::Empty(isolate));
TypeName* new_data =
static_cast<TypeName*>(malloc(length * sizeof(*new_data)));
static_cast<TypeName*>(node::Malloc(length * sizeof(*new_data)));

This comment has been minimized.

@bnoordhuis

bnoordhuis Sep 2, 2016

Member

A future improvement is to replace Malloc calls like this one with a Calloc-like version that does overflow checks (but doesn't zero the memory.)

@bnoordhuis

bnoordhuis Sep 2, 2016

Member

A future improvement is to replace Malloc calls like this one with a Calloc-like version that does overflow checks (but doesn't zero the memory.)

return realloc(pointer, size);
}
// as per spec realloc behaves like malloc if passed nullptr

This comment has been minimized.

@bnoordhuis

bnoordhuis Sep 2, 2016

Member

Can you capitalize and punctuate the comment?

@bnoordhuis

bnoordhuis Sep 2, 2016

Member

Can you capitalize and punctuate the comment?

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Sep 2, 2016

Member

LGTM with a style nit. You don't have to prefix the function calls with node:: but it doesn't hurt either.

Member

bnoordhuis commented Sep 2, 2016

LGTM with a style nit. You don't have to prefix the function calls with node:: but it doesn't hurt either.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Sep 2, 2016

Member

@Trott do you prefer to land your PR or that I land your commit as part of this one ? The later is probably easier for me but its up to you.

Member

mhdawson commented Sep 2, 2016

@Trott do you prefer to land your PR or that I land your commit as part of this one ? The later is probably easier for me but its up to you.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Sep 2, 2016

Member

@mhdawson No preference, so GO FOR IT! (And ping me if you change your mind and want me to do it for whatever reason.)

Member

Trott commented Sep 2, 2016

@mhdawson No preference, so GO FOR IT! (And ping me if you change your mind and want me to do it for whatever reason.)

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 2, 2016

Member

LGTM, maybe use “src:” as the subsystem prefix for the first commit?

Member

addaleax commented Sep 2, 2016

LGTM, maybe use “src:” as the subsystem prefix for the first commit?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 2, 2016

Member

Also, I don’t want to step on @bnoordhuis’ toes or anything, but I like the general idea here… doing all memory management through node:: methods seems nice to me, it seems like it would make it easier to do things like call isolate->LowMemoryNotification() and retry in case malloc() fails?

Member

addaleax commented Sep 2, 2016

Also, I don’t want to step on @bnoordhuis’ toes or anything, but I like the general idea here… doing all memory management through node:: methods seems nice to me, it seems like it would make it easier to do things like call isolate->LowMemoryNotification() and retry in case malloc() fails?

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Sep 2, 2016

Member

I don't want to land this today since its probably one that should get is 72hrs and am out on Monday for the Canadian long weekend. I'll plan to land on Tuesday when I'm back.

Member

mhdawson commented Sep 2, 2016

I don't want to land this today since its probably one that should get is 72hrs and am out on Monday for the Canadian long weekend. I'll plan to land on Tuesday when I'm back.

mhdawson added a commit that referenced this pull request Sep 6, 2016

src: normalize malloc, realloc
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.

- Introduce node::malloc, node::realloc and node::calloc that should
  be used throught our source.
- Update all existing node source files to use the new functions
  instead of the native allocation functions.

Fixes: #7549
PR-URL: #7564
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Sep 6, 2016

Member

landed as a00ccb0

Member

mhdawson commented Sep 6, 2016

landed as a00ccb0

@mhdawson mhdawson closed this Sep 6, 2016

@Fishrock123 Fishrock123 referenced this pull request Sep 6, 2016

Closed

v6.6.0 pre-proposal #8428

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Sep 8, 2016

Member

@mhdawson if you want this on v6.x you'll need to backport unfortunately. I segfaulted when I tried.

Member

Fishrock123 commented Sep 8, 2016

@mhdawson if you want this on v6.x you'll need to backport unfortunately. I segfaulted when I tried.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 9, 2016

Member

@Fishrock123 This needs to land together with ed640ae, master was broken for one commit there. :/ If you experienced problems even with that included, we’re probably in trouble and, while I know that you’re busy, it might be really good to have some debugging info if you can provide any.

Member

addaleax commented Sep 9, 2016

@Fishrock123 This needs to land together with ed640ae, master was broken for one commit there. :/ If you experienced problems even with that included, we’re probably in trouble and, while I know that you’re busy, it might be really good to have some debugging info if you can provide any.

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Sep 9, 2016

Member

Hmmm, ok will retry.

Member

Fishrock123 commented Sep 9, 2016

Hmmm, ok will retry.

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Sep 9, 2016

Member

Looks good with both commits. Thanks!

Member

Fishrock123 commented Sep 9, 2016

Looks good with both commits. Thanks!

Fishrock123 added a commit that referenced this pull request Sep 9, 2016

src: normalize malloc, realloc
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.

- Introduce node::malloc, node::realloc and node::calloc that should
  be used throught our source.
- Update all existing node source files to use the new functions
  instead of the native allocation functions.

Fixes: #7549
PR-URL: #7564
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

 Conflicts:
	src/node.cc

Fishrock123 added a commit that referenced this pull request Sep 9, 2016

src: normalize malloc, realloc
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.

- Introduce node::malloc, node::realloc and node::calloc that should
  be used throught our source.
- Update all existing node source files to use the new functions
  instead of the native allocation functions.

Fixes: #7549
PR-URL: #7564
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

 Conflicts:
	src/node.cc
@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Sep 9, 2016

Member

Great to hear :)

Member

mhdawson commented Sep 9, 2016

Great to hear :)

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 30, 2016

Member

@mhdawson should this be backported to v4?

Member

MylesBorins commented Sep 30, 2016

@mhdawson should this be backported to v4?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Oct 7, 2016

Member

I don’t think this has to be backported, but if it is, it needs to be backported together with #8572 (and maybe, but not necessarily, #8482).

Member

addaleax commented Oct 7, 2016

I don’t think this has to be backported, but if it is, it needs to be backported together with #8572 (and maybe, but not necessarily, #8482).

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Oct 7, 2016

Member

I'm going to say no. We did not see issues in V4.x on AIX as they were introduced with node-inspector (as a side effect) and while this addresses the problem more generally I'd leave 4.X as is until we run into a similar issue in which case we can re-evaluate.

Member

mhdawson commented Oct 7, 2016

I'm going to say no. We did not see issues in V4.x on AIX as they were introduced with node-inspector (as a side effect) and while this addresses the problem more generally I'd leave 4.X as is until we run into a similar issue in which case we can re-evaluate.

addaleax added a commit to addaleax/node that referenced this pull request Nov 22, 2016

src: normalize malloc, realloc
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.

- Introduce node::malloc, node::realloc and node::calloc that should
  be used throught our source.
- Update all existing node source files to use the new functions
  instead of the native allocation functions.

Fixes: nodejs#7549
PR-URL: nodejs#7564
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins added a commit that referenced this pull request Nov 22, 2016

src: normalize malloc, realloc
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.

- Introduce node::malloc, node::realloc and node::calloc that should
  be used throught our source.
- Update all existing node source files to use the new functions
  instead of the native allocation functions.

Fixes: #7549
PR-URL: #7564
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

@MylesBorins MylesBorins referenced this pull request Nov 22, 2016

Merged

v4.7.0 proposal #9736

@mhdawson mhdawson deleted the mhdawson:aix3 branch Mar 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment