core: normalize malloc, realloc #7564
Conversation
nodejs-github-bot
added
c++
|
FYI those involved in the original discussion @bnoordhuis, @jasnell, @ChALkeR |
|
Bleh, I hate this let's-add-more-macros approach. If you want to eradicate |
mscdex
added
lib / src
|
LGTM with green CI but would prefer @bnoordhuis to be happy with it also. |
|
I'll work on moving everything over to new[] / delete[]. |
|
@bnoordhuis is that something you are working now ? Just want to get a feel for whether its the short term fix or not. |
|
Yes. |
|
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). |
|
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. |
|
Ok so does that mean we sill need to land this PR to resolve the current issues on AIX ? |
|
@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 ? |
|
I want to take a slightly different approach. I have some in-progress work that I can PR next week. |
|
Ok, thanks. |
|
Any update ? |
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
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 <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
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 <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
|
@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. |
`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
|
@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. |
|
@mhdawson No preference, so GO FOR IT! (And ping me if you change your mind and want me to do it for whatever reason.) |
|
LGTM, maybe use “src:” as the subsystem prefix for the first commit? |
|
Also, I don’t want to step on @bnoordhuis’ toes or anything, but I like the general idea here… doing all memory management through |
|
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. |
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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
|
landed as a00ccb0 |
|
@mhdawson if you want this on v6.x you'll need to backport unfortunately. I segfaulted when I tried. |
|
@Fishrock123 This needs to land together with ed640ae, |
|
Hmmm, ok will retry. |
|
Looks good with both commits. Thanks! |
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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Conflicts: src/node.cc
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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Conflicts: src/node.cc
|
Great to hear :) |
|
@mhdawson should this be backported to v4? |
|
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. |
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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected 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.