Skip to content

Possible alloc-before-validate leak in SocketWrap::New #94

Description

@OvOhao

Possible alloc-before-validate leak in SocketWrap::New

I found a possible alloc-before-validate leak (CWE-401) in the SocketWrap::New constructor
callback. A SocketWrap C++ object is heap-allocated with new at the very top of the method,
but several argument-validation paths throw and return before socket->Wrap(info.This())
ties the native object to a GC-managed JS wrapper. Because Wrap never runs on those paths, the
object is never adopted by V8 and its destructor never fires, so the SocketWrap is leaked on
every rejected call. On the CreateSocket()-failure path the leak is potentially worse:
CreateSocket may have already opened a raw socket fd (and possibly fcntl'd it) before returning
non-zero. Note, however, that even if ~SocketWrap did run it would not reliably close that fd —
CloseSocket() only closes poll_fd_ if (this->poll_initialised_), and poll_initialised_ is
set to true only near the end of CreateSocket; a failure before that point leaves the fd open
with poll_initialised_ == false, so the descriptor is unrecoverable regardless of the wrapper
leak.

File: src/raw.cc

Function: SocketWrap::New

SocketWrap* socket = new SocketWrap ();
int rc, family = AF_INET;

if (info.Length () < 1) {
    Nan::ThrowError("One argument is required");
    return;                                  // <-- socket leaked
}
if (! info[0]->IsUint32 ()) {
    Nan::ThrowTypeError("Protocol argument must be an unsigned integer");
    return;                                  // <-- socket leaked
}
...
rc = socket->CreateSocket ();
if (rc != 0) {
    Nan::ThrowError(raw_strerror (rc));
    return;                                  // <-- socket (and any opened fd) leaked
}

socket->Wrap (info.This ());                 // ownership handed to GC only here
  1. new SocketWrap() allocates the native object; at this point nothing owns it — it is not yet
    wrapped, so V8 GC cannot reclaim it.
  2. Each early return after a Nan::Throw* (missing arg, non-Uint32 protocol, non-Uint32
    family, and CreateSocket() != 0) exits the function without calling Wrap and without
    delete socket, leaking the object.
  3. On the CreateSocket() != 0 branch, CreateSocket can fail after socket() succeeded (e.g.
    a subsequent fcntl fails), leaving poll_fd_ an open descriptor. Because CloseSocket()
    guards its close with if (this->poll_initialised_) and poll_initialised_ is not yet set on
    an early CreateSocket failure, this fd would leak even if the destructor ran — so the fix
    must also close the fd on the failure path, not merely delete socket.

JS trigger (if applicable):

raw.SocketWrap is not exported from the package entry, so the leak is triggered through the
public factory, which internally does new raw.SocketWrap(...) and hits the throw-after-new
path when the constructor validation fails:

const raw = require('raw-socket');
// Each call passes an invalid protocol string to `new raw.SocketWrap(...)` internally,
// which throws in SocketWrap::New after allocation -> one SocketWrap leaked per call.
for (let i = 0; i < 100000; i++) {
  try { raw.createSocket({ protocol: 'bad' }); } catch (e) {}
}

Suggested fix: delete socket; before each early return (and explicitly close any fd opened by a
partially-successful CreateSocket), or (cleaner) validate all arguments before allocating, or
adopt the raw pointer into a std::unique_ptr released only at the successful Wrap call.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions