Skip to content
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

src: temporary fix for AIX malloc issue #7229

Closed
wants to merge 1 commit into from
Closed

src: temporary fix for AIX malloc issue #7229

wants to merge 1 commit into from

Conversation

imran-iq
Copy link
Contributor

@imran-iq imran-iq commented Jun 8, 2016

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

src

Description of change

With the introduction of the v8 inspector the following tests started to
fail on AIX:
test-async-wrap-post-did-throw
test-async-wrap-throw-from-callback
test-crypto-random

The reason for the failures was a malloc(0) issue. On GNU compatible
malloc a malloc(0) returns a vaild pointer, but on AIX it returns
a null pointer which is considered to be a failure in node.

In order to get a GNU compatible malloc AIX requires the following
flags to be defined:
_LINUX_SOURCE_COMPAT
_ALL_SOURCE

However regardless of these flags, the STL headers undefine malloc
and replace it with a non GNU compatible malloc. This is a temporary
workaround until the AIX team for gcc release an update that fixes this
behaviour.

For more information (and also fixes): #7080

AIX CI run: https://ci.nodejs.org/job/node-test-commit-aix/210/

Sorry, something went wrong.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol labels Jun 8, 2016
@imran-iq
Copy link
Contributor Author

imran-iq commented Jun 8, 2016

note once the compiler fix has been delivered and gcc updated on the build machines I will revert this change

@mhdawson mhdawson self-assigned this Jun 8, 2016
@mhdawson
Copy link
Member

mhdawson commented Jun 8, 2016

LGTM provided CI is green.

@imran-iq
Copy link
Contributor Author

imran-iq commented Jun 8, 2016

uv_buf_t* buf = reinterpret_cast<uv_buf_t*>(malloc(sizeof(uv_buf_t)));
uv_write_t* req = reinterpret_cast<uv_write_t*>(malloc(sizeof(uv_write_t)));
uv_buf_t* buf = reinterpret_cast<uv_buf_t*>(NODE_MALLOC(sizeof(uv_buf_t)));
uv_write_t* req = reinterpret_cast<uv_write_t*>(NODE_MALLOC(sizeof(uv_write_t)));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is > 80 chars, not too sure what a clean way to break it would be. Any suggestions?
Same with line 478.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other places in the codebase there’s a line break (+ double indent) after the = in these situations

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated accordingly!

@imran-iq
Copy link
Contributor Author

imran-iq commented Jun 8, 2016

CI is all green minus the linting issues which I have already fixed up

@jasnell
Copy link
Member

jasnell commented Jun 10, 2016

LGTM

@mhdawson
Copy link
Member

Ok , will plan to land this tomorrow unless there objections before then.

@ChALkeR
Copy link
Member

ChALkeR commented Jun 14, 2016

Uhm. What exactly does call malloc(0)? Can we somehow fix the cause of the issue, and not rely on malloc implementation details? Per standard, malloc(0) behaviour is implementation-defined, we shouldn't be calling that in our own source code.

See also #6305.

@mhdawson
Copy link
Member

This is the specific example:

bash-4.3$ cat test.js
var crypto = require('crypto');
crypto.randomBytes(0);

That we know of in this case. There could be others which are possible through the APIs but which are not caught by the existing tests.

We could fix this specific case if that make sense.

@mhdawson
Copy link
Member

@iwuzhere could you put together the patch for fixing just the cases were w know the malloc(0) was being hit so that we can see how intrusive it is (or not)

With the introduction of the v8 inspector the following tests started to
fail on AIX:
test-async-wrap-post-did-throw
test-async-wrap-throw-from-callback
test-crypto-random

The reason for the failures was a `malloc(0)` issue. On GNU compatible
`malloc` a `malloc(0)` returns a vaild pointer, but on AIX it returns
a null pointer which is considered to be a failure in node.

In order to get a GNU compatible `malloc` AIX requires the following
flags to be defined:
`_LINUX_SOURCE_COMPAT`
`_ALL_SOURCE`

However regardless of these flags, the STL headers undefine `malloc`
and replace it with a non GNU compatible `malloc`. This is a temporary
workaround until the AIX team for gcc release an update that fixes this
behaviour.
@imran-iq
Copy link
Contributor Author

imran-iq commented Jun 23, 2016

a simple non-intrusive fix would be the following:

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index 383a18a..1a40dd1 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -5490,6 +5490,11 @@ void RandomBytes(const FunctionCallbackInfo<Value>& args) {
     return env->ThrowRangeError("size is not a valid Smi");

   Local<Object> obj = env->NewInternalFieldObject();
+
+  if (size == 0) {
+    size++;
+  }
+
   RandomBytesRequest* req = new RandomBytesRequest(env, obj, size);

   if (args[1]->IsFunction()) {

Again this raises the question if crypto.randomBytes(0); is something that should be supported? Like in what cases do we need 0 random bytes?

@bnoordhuis
Copy link
Member

Again this raises the question if crypto.randomBytes(0); is something that should be supported?

Wrong question. If it works now, then it shouldn't be broken because of some platform-specific quirk.

@imran-iq
Copy link
Contributor Author

imran-iq commented Jul 4, 2016

Well it seems like this solution is not going to be accepted (it is somewhat hack-y). The real issue is with gcc on AIX. There /should/ be an update soon fixing the issue.
As of right now, the tests are not failing because of 624734e
Closing this PR.

@mhdawson
Copy link
Member

mhdawson commented Jul 5, 2016

Possibly failing again, see #7549

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants