Skip to content

Possible BatchWorker leak when a batchWrite operation fails validation #227

Description

@OvOhao

Possible BatchWorker leak when a batchWrite operation fails validation

I found a possible native heap leak in EnvWrap::batchWrite. The heap BatchWorker
(together with its actions array, its Nan::Callbacks, and every value already
saved into its persistent) is allocated up front, but several per-operation
validation errors return from the middle of the loop before the worker is queued,
so nothing ever frees it.

File: src/env.cpp

Function: EnvWrap::batchWrite

The worker and its owned resources are allocated before the operation loop:

action_t* actions = new action_t[length];
...
callback = new Nan::Callback(v8::Local<v8::Function>::Cast(info[2]));   // or info[1]
...
BatchWorker* worker = new BatchWorker(
    ew->env, actions, length, putFlags, callback, progress
);

BatchWorker (a Nan::AsyncProgressWorker) owns all of this and frees it in its
destructor — which only runs after the worker is queued and completes:

~BatchWorker() {
    ...
    delete[] actions;
}

But the per-operation loop has several early returns that run after worker exists
and before Nan::AsyncQueueWorker(worker):

for (unsigned int i = 0; i < array->Length(); i++) {
    ...
    keyType = inferAndValidateKeyType(key, options, dw->keyType, keyIsValid);
    if (!keyIsValid) {
        return;                                                        // <-- worker leaked
    }
    action->freeKey = argToKey(key, action->key, keyType, keyIsValid);
    if (!keyIsValid) {
        return;                                                        // <-- worker leaked
    }
    ...
    } else {
        return Nan::ThrowError("The ifValue must be a buffer or null/undefined.");  // <-- worker leaked
    }
    ...
    return Nan::ThrowError("The ifDB must be a database object or null/undefined."); // <-- worker leaked
    ...
    return Nan::ThrowError("The value must be a buffer or null/undefined.");         // <-- worker leaked
}
...
worker->SaveToPersistent("env", info.This());
Nan::AsyncQueueWorker(worker);   // only here does the worker become owned/freeable

On any of these error branches the heap BatchWorker — and with it the actions
array, the callback/progress Nan::Callbacks, and every key/value already
passed to worker->SaveToPersistent(...) — is never queued and never deleted, so it
leaks. Each one is reachable from JavaScript by passing a batchWrite operation with
an invalid key, a non-buffer ifValue/value, or a bad ifDB.

Suggested fix: delete worker; before each early return in the loop (the worker's
destructor frees actions and the callbacks), or validate every operation before
allocating the worker.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions