src: return static buffer on malloc(0)

[mscdex]

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)

• src

Description of change

This is an alternate approach to #8572 that reuses a static buffer for malloc(0) requests to restore pre-node v6.6.0 behavior. This requires the addition of a new node::Free() for all buffers created via node::Malloc()/node::Calloc().

Fixes: #8571

[mscdex]

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

[yorkie]

LGTM

[bnoordhuis]

It might put people on the wrong foot because it deviates from what normal malloc does. malloc(0) is allowed to either return nullptr or a unique pointer but not a fixed pointer.

If you think performance is at stake, then it's arguably better to teach callers to handle zero-sized allocations explicitly.

[bnoordhuis]

Oh, another thing: there are at least one or two places in the source code where we pass a heap-allocated buffer to an external library (openssl), with the library responsible for freeing the memory again. I don't think we normally pass zero-sized buffers but it would fail badly if we did.

[mscdex]

@bnoordhuis Handling the nullptr in node code is fine, but there are some external APIs (e.g. OpenSSL) that actually check the pointer first and may return early for example. That would break compatibility with node v6 pre-v6.6.0, because then we would start returning errors instead of a result.

Allowing a nullptr to be passed to OpenSSL is probably the right thing to do going forward though (albeit in node v7 or later).

[addaleax]

Are you sure this works as it is? Having fake_mem as a static variable means that every object file is going to end up with its own instance of that, meaning that e.g. the Free() from node_crypto.cc can’t handle Malloc()ed buffers from node_buffer.cc.

[mscdex]

Changed. Does it look better now?

[addaleax]

@mscdex Yeah, that should work, but it may come with a bit of overhead and it’s not technically thread-safe.

Something that might be a bit faster would be using a compile-time static buffer, i.e. extern char fake_mem[1]; in util-inl.h and char fake_mem[1]; in util.cc, because that should enable the compiler to inline the memory address (which may become an actual constant).

[mscdex]

Updated.

[mscdex]

CI again after recent changes: https://ci.nodejs.org/job/node-test-pull-request/4150/

[addaleax]

LGTM

[jasnell]

LGTM

[bnoordhuis]

I'm going to go out on a limb and say this does not LGTM. The potential for subtle bugs outweighs any benefits, IMO.

[jasnell]

That's fair. Is this a "don't do this at all" or a "there's a better way" type of thing?

[addaleax]

I think the “better way” would basically be #8572? I’m reopening that because it seems like a solution that can get to consensus pretty easily, and anything else can be discussed later.