src: Malloc/Calloc size 0 returns non-null pointer

[Trott]

Checklist

• make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

crypto

Description of change

crypto.pbkdf2() with empty password and/or salt causes a fatal error in
Node.js 6.6.0. It did not in 6.5.0. The problematic change is
a00ccb0. We still need to review other
changes in that change set, but this is a test and fix for the specific
issue reported in #8571.

The problem is that malloc(0) may return NULL on some platforms. So
do not report out-of-memory error unless malloc was passed a number
greater than 0.

Fixes: #8571

[Trott]

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

[mscdex]

I should point out that there's also a behavior change here, since before the introduction of node::Malloc(), a Buffer would actually get passed to the crypto.pbkdf2() callback, and now with this PR it would get passed an Error instead.

[Trott]

There's also a behavior change

Yes, that's correct. I didn't consider that. Thanks for pointing it out.

I wonder if the best path is to revert a00ccb0 in the 6.x line and release 6.6.1 soon.

Meanwhile, leave a00ccb0 along with this change in 7.x. And of course review the other instances of nullptr checks...

[yorkie]

if (passlen > 0 && pass = static_cast<char*>(node::Malloc(passlen))) {
  if (pass == nullptr) {
    FatalError("node::PBKDF2()", "Out of Memory");
  }
}

If passlen is 0, node::Malloc() could be skipped?

[mscdex]

The if (pass == nullptr) branch would never execute since you're already testing the "truthiness" of pass up above. Besides, I'm not sure that the "assignment and test" inside a conditional is really encouraged in node core (at least in C++).

At any rate, I don't see anything wrong in doing it the way it's currently being done.

[yorkie]

@mscdex The below block seems to be more clear as you concerned.

if (passlen > 0) {
  pass = static_cast<char*>(node::Malloc(passlen));
  if (pass == nullptr) {
    FatalError("node::PBKDF2()", "Out of Memory");
  }
}

And for this question,

At any rate, I don't see anything wrong in doing it the way it's currently being done.

Yes, the current being done is not wrong in anything, this is just a suggestion, when passlen is 0 or less than that, we could decrease calls to node::Malloc() by what I did propose :)

But I'm not sure if the compiler will optimize this or this is valuable to make this change.

[mscdex]

One benefit to calling node::Malloc() is that you're sure to either get a null or valid pointer. In most places we probably initialize the variable storing the allocated memory to nullptr, but in a way node::Malloc() could be seen as an additional safety precaution (in case the variable should be uninitialized or set to some other previous pointer)?

[yorkie]

@mscdex Thanks for your detailed explanation, after I did read src/util-inl.h#L232-L245, I would think should we actually return a specific unique pointer for zero-sized allocation requests to replace the current one? Thus we don't need any changes on our previous check by nullptr.

Introducing an extra normalization to return nullptr is creating a conflicts with our previous checks.

[mscdex]

I thought about that too and that could be useful, but I'm not sure how everyone else would feel about it.

[bnoordhuis]

I wonder if the best path is to revert a00ccb0 in the 6.x line and release 6.6.1 soon.

The code that it replaced was broken anyway because it relied on implementation-specific behavior.

If you are worried about regressions, a better fix is to avoid zero-sized allocations like this:

diff --git a/src/util-inl.h b/src/util-inl.h
index 5644ee9..31411bb 100644
--- a/src/util-inl.h
+++ b/src/util-inl.h
@@ -246,11 +246,13 @@ void* Realloc(void* pointer, size_t size) {

 // As per spec realloc behaves like malloc if passed nullptr.
 void* Malloc(size_t size) {
+  if (size == 0) size = 1;
   return Realloc(nullptr, size);
 }

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

[addaleax]

I’m marking #8482 as blocked while this discussion is ongoing. This change LGTM as it is, and I actually do like @yorkie’s idea, too. If it still makes sense, I’d definitely want to implement that on top of #8482.

[Trott]

I think I like @bnoordhuis's suggestion above best. That would unblock #8482 as well? Or not quite?

[Trott]

Updated with @bnoordhuis's suggestion for a more complete fix. The test added here works in 6.5.0, fails in 6.6.0, and passes again with the change here, all as expected. PTAL /cc @mhdawson

So that's a fix for the 6.x line. I guess the next question is whether we want behavior to be unchanged in 7.x or if we want the crypto library to emit an error event in this situation. If there's no compelling reason to make a breaking change like that in v7, then I guess this is good for both lines...

[Trott]

By the way, is there an existing reasonable place to drop a C++ test that calls Malloc() and Calloc() with zeroes and confirms that it does not get a null pointer? (You'd think I'd know the answer to that question....)

[yorkie]

@Trott Shouldn't we open another pull-request to change Malloc/Calloc functions and merge it ASAP before this gets merged? This one seems not KISS enough :(

[Trott]

@yorkie The only thing I see that's optional is the var->const and equal()->strictEqual() changes. I'll pull those out to make this as minimal as possible.

[Trott]

Removed superfluous style changes, edited commit message, squashed, force pushed. PTAL.

[Trott]

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

[yorkie]

@Trott What I did mean is that you did write in the commit message as:

Change Malloc()/Calloc() so that size zero does not return a null
pointer, consistent with prior behavior.

This change seems a side-effect and it'd better to land in another PR? Of course this is a friendly suggestion, both are reasonable :)

[bnoordhuis]

is there an existing reasonable place to drop a C++ test that calls Malloc() and Calloc() with zeroes and confirms that it does not get a null pointer?

test/cctest/util.cc

[addaleax]

This change seems a side-effect and it'd better to land in another PR? Of course this is a friendly suggestion, both are reasonable :)

Maybe just split into two commits? And don’t worry about #8482, I’ll rebase on whatever comes out of this change.

[mscdex]

What does everyone think about just returning a static 1-byte sized buffer on request of a zero-length chunk of memory? That way we definitely only allocate a 1-byte sized buffer once.

[addaleax]

@mscdex That’s basically @yorkie’s idea from above, right? I’d still be on board with that.

[mscdex]

@addaleax Basically, yes.

[Trott]

If someone else wants to take over this PR to add the "only allocate a pointer once" business (file a separate PR or push a change to this branch or whatever), that would be great.

(Feel free to just take any of the 12 lines of code here that's useful to you, nearly half of which I didn't write anyway. :-D )

[mscdex]

Now that I think about it, there would be more work involved if we used a static buffer to make sure no call to free() happens on it. This would require either prefixing every free() with a conditional or adding a node::Free() that replaces free() calls. In the latter case we could then just easily compare pointers to know whether to actually call free or not.

[mscdex]

I submitted the alternate solution in #8658.

[yorkie]

+1 on @mscdex's patch :)

[Trott]

I'm going to close this in favor of #8658. If anyone thinks that's a mistake for whatever reason, feel free to comment or re-open.

[addaleax]

Still LGTM! :)

[mhdawson]

Sorry for the late response, just catching up from being away. LGTM from me.

[Trott]

Landed in d2eb7ce