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

crypto: make malloc failure check cross-platform #8352

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@Trott
Member

Trott commented Aug 31, 2016

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

crypto

Description of change

malloc(0) may return NULL on some platforms. Do not report
out-of-memory error unless malloc was passed a number greater than
0.

Marking as in progress for the moment. This is an attempt to fix some of the perma-flaky tests on AIX.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 31, 2016

Member

This seems to work. Removing in progress label. /cc @mhdawson @nodejs/crypto

Hopefully this is an acceptable short term workaround if it is not acceptable as a permanent fix.

Guess I better start looking around the code to see if anything bad could happen to that null pointer...

Member

Trott commented Aug 31, 2016

This seems to work. Removing in progress label. /cc @mhdawson @nodejs/crypto

Hopefully this is an acceptable short term workaround if it is not acceptable as a permanent fix.

Guess I better start looking around the code to see if anything bad could happen to that null pointer...

@Trott Trott removed the in progress label 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: #7564
PR-URL: #8352
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 31, 2016

Member

Fixes three of the four current perma-flaky tests on AIX. SmartOS failure looks unrelated.

CI again: https://ci.nodejs.org/job/node-test-pull-request/3901/

Member

Trott commented Aug 31, 2016

Fixes three of the four current perma-flaky tests on AIX. SmartOS failure looks unrelated.

CI again: https://ci.nodejs.org/job/node-test-pull-request/3901/

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Aug 31, 2016

Member

See also #7564. I wasn't very happy with that approach but, mea culpa, I didn't get around to posting the alternative solution I talked about. I suppose any fix is better than no fix at this point.

Member

bnoordhuis commented Aug 31, 2016

See also #7564. I wasn't very happy with that approach but, mea culpa, I didn't get around to posting the alternative solution I talked about. I suppose any fix is better than no fix at this point.

@indutny

This comment has been minimized.

Show comment
Hide comment
@indutny

indutny Aug 31, 2016

Member

LGTM, if it works

Member

indutny commented Aug 31, 2016

LGTM, if it works

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Aug 31, 2016

Member

LGTM

Member

addaleax commented Aug 31, 2016

LGTM

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 31, 2016

Member

LGTM

Member

jasnell commented Aug 31, 2016

LGTM

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Aug 31, 2016

Member

I'd prefer to land #7564 as it addresses the general problem as opposed to the specific problem that this patch addresses. However, I'm not opposed to landing this as well as it would be better not to depend on the platform specific malloc(0) behavior in this specific case.

So LTGM to land this but @bnoordhuis can you also give me an LTGM on #7564 as well so that I could land it.

Member

mhdawson commented Aug 31, 2016

I'd prefer to land #7564 as it addresses the general problem as opposed to the specific problem that this patch addresses. However, I'm not opposed to landing this as well as it would be better not to depend on the platform specific malloc(0) behavior in this specific case.

So LTGM to land this but @bnoordhuis can you also give me an LTGM on #7564 as well so that I could land it.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Sep 1, 2016

Member

Would I be correct to infer that folks providing LGTM comments are affirming that they believe using the null pointer will have no detrimental effects on security or stability? (That's my opinion, but I don't trust myself to analyze C++ crypto code, so I'm looking for specific external validation on this.)

Member

Trott commented Sep 1, 2016

Would I be correct to infer that folks providing LGTM comments are affirming that they believe using the null pointer will have no detrimental effects on security or stability? (That's my opinion, but I don't trust myself to analyze C++ crypto code, so I'm looking for specific external validation on this.)

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 1, 2016

Member

@Trott Yes, using nullptr for a zero-length Buffer should be okay. We’re definitely having tests to make sure that that works. :)

Member

addaleax commented Sep 1, 2016

@Trott Yes, using nullptr for a zero-length Buffer should be okay. We’re definitely having tests to make sure that that works. :)

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Sep 1, 2016

Member

Based on the way that @bnoordhuis suggested that I change the implementation we'll need this fix as it moves all platforms so that we'll consistently get a nullptr for malloc(0).

To be able to test I cherry picked this across/resolved merge issues so might make sense to land them together. Just about to push 2 commits to #7564 and then do a CI run to make sure all it ok across platforms.

Member

mhdawson commented Sep 1, 2016

Based on the way that @bnoordhuis suggested that I change the implementation we'll need this fix as it moves all platforms so that we'll consistently get a nullptr for malloc(0).

To be able to test I cherry picked this across/resolved merge issues so might make sense to land them together. Just about to push 2 commits to #7564 and then do a CI run to make sure all it ok across platforms.

mhdawson added a commit to mhdawson/io.js that referenced this pull request Sep 1, 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

mhdawson added a commit that referenced this pull request Sep 6, 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`.

PR-URL: #8352
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Sep 6, 2016

Member

Landed as ed640ae

Member

mhdawson commented Sep 6, 2016

Landed as ed640ae

@mhdawson mhdawson closed this Sep 6, 2016

@Fishrock123 Fishrock123 referenced this pull request Sep 6, 2016

Closed

v6.6.0 pre-proposal #8428

Fishrock123 added a commit that referenced this pull request Sep 9, 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`.

PR-URL: #8352
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Fishrock123 added a commit that referenced this pull request Sep 9, 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`.

PR-URL: #8352
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 30, 2016

Member

@mhdawson should this be backported?

Member

MylesBorins commented Sep 30, 2016

@mhdawson should this be backported?

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Oct 11, 2016

Member

@Trott backport?

edit: does not land cleanly

Member

MylesBorins commented Oct 11, 2016

@Trott backport?

edit: does not land cleanly

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Oct 11, 2016

Member

@thealphanerd I believe it is only needed if and only if https://github.com/nodejs/node/commit/a00ccb0fb9eb716925058b0a20fcec9251de3309/ / #7564 lands. That PR is currently marked as "do not land" so I think this too should be marked that way.

Member

Trott commented Oct 11, 2016

@thealphanerd I believe it is only needed if and only if https://github.com/nodejs/node/commit/a00ccb0fb9eb716925058b0a20fcec9251de3309/ / #7564 lands. That PR is currently marked as "do not land" so I think this too should be marked that way.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Oct 11, 2016

Member

Agreed.

Member

mhdawson commented Oct 11, 2016

Agreed.

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