HTTP/2 threaded sendfile reentry races shared connection task context

[xintenseapple]

Summary

When freenginx is built with --with-threads --with-http_v2_module and configured with aio threads; plus sendfile on;, HTTP/2 output can re-enter the threaded sendfile path while the connection's existing sendfile task is still active. The reentrant call overwrites the shared task context before the worker thread has finished reading it.

Affected Area

src/os/unix/ngx_linux_sendfile_chain.c

The affected path is:

• ngx_linux_sendfile_thread() writes ctx->file, ctx->socket, and ctx->size.
• ngx_linux_sendfile_thread_handler() reads those fields from the worker thread.
• HTTP/2 reaches this path through ngx_http_v2_send_output_queue().

This affects configurations using threaded AIO sendfile for HTTP/2 responses.

Steps to Reproduce

1. Build freenginx with ThreadSanitizer and HTTP/2/thread support:

   ./configure \
     --with-threads \
     --with-http_v2_module \
     --with-debug \
     --without-http_rewrite_module \
     --with-cc-opt='-g -O1 -fno-omit-frame-pointer -fsanitize=thread' \
     --with-ld-opt='-fsanitize=thread'
   make

2. Run with a configuration equivalent to:

   worker_processes 1;
   
   events {
       worker_connections 4096;
       accept_mutex off;
   }
   
   thread_pool default threads=1 max_queue=65536;
   
   http {
       aio threads;
       sendfile on;
       tcp_nopush off;
       http2_chunk_size 16k;
   
       server {
           listen 127.0.0.1:8080 http2;
           root html;
       }
   }

3. Place several large static files under html/.

4. Fetch them concurrently over a single HTTP/2 connection, for example with h2c:

   curl --http2-prior-knowledge \
     --parallel \
     --parallel-max 4 \
     --silent \
     --show-error \
     --fail \
     -o out-0.bin http://127.0.0.1:8080/file-0.bin \
     -o out-1.bin http://127.0.0.1:8080/file-1.bin \
     -o out-2.bin http://127.0.0.1:8080/file-2.bin \
     -o out-3.bin http://127.0.0.1:8080/file-3.bin

Expected Behavior

A connection-level threaded sendfile task should not have its context modified while the worker thread may still be reading it. Reentrant HTTP/2 send attempts should wait for the active sendfile task to complete, or otherwise use synchronization that prevents concurrent access to the shared context.

Actual Behavior

ThreadSanitizer reports data races between the event-loop thread overwriting the shared sendfile context and the worker thread reading it.

Example reports:

WARNING: ThreadSanitizer: data race

Write in ngx_linux_sendfile_thread:
  src/os/unix/ngx_linux_sendfile_chain.c:383
  ctx->file = file;

Previous read in ngx_linux_sendfile_thread_handler:
  src/os/unix/ngx_linux_sendfile_chain.c:408
  file = ctx->file;

HTTP/2 stack:
  ngx_linux_sendfile
  ngx_linux_sendfile_chain
  ngx_http_v2_send_output_queue

Additional reports were observed for:

src/os/unix/ngx_linux_sendfile_chain.c:384
  ctx->socket = c->fd;

src/os/unix/ngx_linux_sendfile_chain.c:385
  ctx->size = size;

racing with worker-thread reads at:

src/os/unix/ngx_linux_sendfile_chain.c:413
  sendfile(ctx->socket, file->file->fd, &offset, ctx->size);

In a short HTTP/2 ThreadSanitizer run with four concurrent streams, three direct sendfile-context races were reported.

Impact

The sendfile task context is per connection, while HTTP/2 can multiplex multiple response streams over that connection. If the race is won at the wrong point, the worker thread may use a file pointer, socket, or byte count from a different send attempt than the one whose DATA frame is currently being emitted.

The most likely impact is response corruption or HTTP/2 stream confusion for configurations using aio threads;, sendfile on;, and HTTP/2. In deployments where different origins or trust domains are coalesced onto the same HTTP/2 connection, response-body confusion could become security-relevant.

Evidence

The relevant code writes the shared task context before posting the task:

ctx->file = file;
ctx->socket = c->fd;
ctx->size = size;

wev->complete = 0;

if (file->file->thread_handler(task, file->file) != NGX_OK) {
    return NGX_ERROR;
}

The worker thread later reads the same fields without synchronization:

file = ctx->file;
offset = file->file_pos;

n = sendfile(ctx->socket, file->file->fd, &offset, ctx->size);

The HTTP copy filter already acknowledges that reentrant sendfile can occur, including under HTTP/2:

/*
 * tolerate sendfile() calls if another operation is already
 * running; this can happen due to subrequests, multiple calls
 * of the next body filter from a filter, or in HTTP/2 due to
 * a write event on the main connection
 */

However, the sendfile context is overwritten before the active-task condition prevents a second post.

Suggested Fix

Before writing ctx->file, ctx->socket, or ctx->size, check whether the connection's sendfile task is already active. If it is active, return NGX_DONE without modifying the context, so the caller waits for the existing threaded sendfile operation to complete.

A minimal fix shape is:

if (task->event.active) {
    ngx_log_debug0(NGX_LOG_DEBUG_CORE, c->log, 0,
                   "linux sendfile thread task already active");

    return NGX_DONE;
}

ctx->file = file;
ctx->socket = c->fd;
ctx->size = size;

[mdounin]

Thanks for reporting this. This looks like a valid bug in the threaded sendfile code. Probably not causing any issues in practice though, since the values overwritten are essentially the same, and sendfile is hardly usable with HTTP/2 anyway (and, more generally, sendfile in threads have other known issues).

I can't say I like the suggested fix though, as it makes duplicate task protection in ngx_thread_task_post() useless. Though might be the way to go after all.