httpsys crashes node.exe process when serving static files

[gimelfarb]

httpsys native module crashes with Access Violation (0xc0000005) when serving a static file over a certain size (at 30KB+), and takes down node.exe process.

Reproducing is fairly simple and happens every time:

var http = require('httpsys').http(),
      connect = require('connect');

http.createServer(connect().use(connect.static(__dirname)));
http.listen(process.env.PORT || 3103);

[gimelfarb]

I have a fix and will be submitting a pull request for this. Update: pull request is #41. Apologies about the 2 identical commits below, I am still learning how to use git properly ;) The 4edc952 can be ignored.

[gimelfarb]

Now for the explanation of the problem and the fix.

The crash happens in the native module httpsys.cc line 1490:

void httpsys_write_callback(uv_async_t* handle, int status)
{
        ...

        // Successful completion 

        if (0 == status)
        {
            // Call completed asynchronously - send notification to JavaScript.

            Handle<Object> event = httpsys_create_event(uv_httpsys, HTTPSYS_WRITTEN);
            httpsys_make_callback(event);
        }

        if (uv_httpsys->lastChunkSent)
        {
            // Response is completed - clean up resources
            httpsys_free(uv_httpsys, FALSE);   // <-- !!!!!!!!!!!! CRASH HERE !!!!!!!!!
            uv_httpsys = NULL;
        }
}

This happens because of a combination of asynchronous completion and code's re-entrancy model. When the last chunk of data is sent back as a response through HttpSendResponseEntityBody (in httpsys_write_body(...) function), I/O completion callback httpsys_write_callback (above) is executed as per usual, and it calls back to JavaScript via HTTPSYS_WRITTEN event, which maps to Socket.prototype._on_written (via Server._dispatch and Server.prototype._on_written).

At that point, writing of the response stream had long finished, and caller would have indicated end of the stream by calling Socket.prototype.write(null, null, true), which would have called Socket.prototype._queue_body_chunk(...). But that last call to write is not a "chunk", but rather an end-of-stream indication. Code here allocates a new this._requestContext.chunks array, but leaves it empty, because chunk is null. Thus we now have a queue, but it is empty!

Back to _on_written callback:

    if (!this._closed && this._requestContext.chunks)
        this._initiate_send_next();

It sees the queue as not empty and initiates the next send, which in turn calls httpsys_write_body(...) native function. Native function gets the queued chunks - which are empty - and therefore constructs an empty buffer to send:

    hr = HttpSendResponseEntityBody(
        uv_httpsys->uv_httpsys_server->requestQueue,
        uv_httpsys->requestId,
        flags,
        uv_httpsys->chunk.FromMemory.pBuffer ? 1 : 0,
        uv_httpsys->chunk.FromMemory.pBuffer ? &uv_httpsys->chunk : NULL,
        NULL,
        NULL,
        0,
        &uv_httpsys->uv_async->async_req.overlapped,
        NULL);

    if (NO_ERROR == hr)
    {
        // Synchronous completion. 

        httpsys_write_callback(uv_httpsys->uv_async, 1);
    }
    else 
    {
        ErrorIf(ERROR_IO_PENDING != hr, hr);
    }

Now, because the buffer is empty, http.sys API decides to be clever, and completes synchronously in this case (i.e. it has nothing to send anyway, plus it had already sent the whole response according to Content-Length header, so as far as it's concerned we're done). So the return code here is NO_ERROR, which means the code recurses into httpsys_write_callback.

Remember from the top of this rant that we are already inside httpsys_write_callback higher up on the call stack, from the previous async I/O completion callback (we still haven't returned). Look at that code again - since uv_httpsys->lastChunkSet is true at this point, it thinks we're done, and tries to cleanup by freeing uv_httpsys memory. Now uv_httpsys is de-allocated.

We now proceed to unwind the stack by returning from all these callbacks, until we are back where we originally started in the httpsys_write_callback native function (after httpsys_make_callback line). But now uv_httpsys is no longer a valid pointer, because memory has been deallocated, and it points at garbage!

Since garbage is likely to be non-zero, the if test succeeds, and the code proceeds to free(uv_httpsys) again, which of course fails with a spectacular crash of the node.exe process! Finita la commedia.

FIX: The fix is to stop this recursion on the first _on_written callback in Javascript code, and avoid calling _initiate_send_next() when we know the chunks array is empty and it will complete synchronously. So we postpone it until the next loop iteration (process.nextTick) to unwind the native stack so that there is no chance of it accessing de-allocated memory when we drop back into it.