NULL dereference in ngx_http_upstream.c

[Antony Dovgal]

Nginx workers started to crash recently with the following backtrace:

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x0000000000506193 in ngx_http_upstream_cache_background_update (u=0x85e2450, r=0x864f640) at src/http/ngx_http_upstream.c:1102
1102    src/http/ngx_http_upstream.c: No such file or directory.
(gdb) bt
#0  0x0000000000506193 in ngx_http_upstream_cache_background_update (u=0x85e2450, r=0x864f640) at src/http/ngx_http_upstream.c:1102
#1  ngx_http_upstream_init_request (r=0x864f640) at src/http/ngx_http_upstream.c:598
#2  0x00000000004f5177 in ngx_http_read_client_request_body (r=r@entry=0x864f640, post_handler=0x503710 <ngx_http_upstream_init>)
    at src/http/ngx_http_request_body.c:77
#3  0x000000000053cde9 in ngx_http_proxy_handler (r=0x864f640) at src/http/modules/ngx_http_proxy_module.c:973
#4  0x00000000004e4c1f in ngx_http_core_content_phase (r=0x864f640, ph=<optimized out>) at src/http/ngx_http_core_module.c:1176
#5  0x00000000004e350d in ngx_http_core_run_phases (r=0x864f640) at src/http/ngx_http_core_module.c:865
#6  ngx_http_handler (r=r@entry=0x864f640) at src/http/ngx_http_core_module.c:848
#7  0x00000000004ebd17 in ngx_http_process_request (r=0x864f640) at src/http/ngx_http_request.c:1954
#8  0x00000000004ecff0 in ngx_http_process_request_line (rev=0x7f24ba0e4fb0) at src/http/ngx_http_request.c:1054
#9  0x00000000004d2921 in ngx_epoll_process_events (cycle=0x34f8410, timer=<optimized out>, flags=<optimized out>)
    at src/event/modules/ngx_epoll_module.c:902
#10 0x00000000004c7d08 in ngx_process_events_and_timers (cycle=cycle@entry=0x34f8410) at src/event/ngx_event.c:242
#11 0x00000000004d0025 in ngx_worker_process_cycle (cycle=0x34f8410, data=<optimized out>) at src/os/unix/ngx_process_cycle.c:750
#12 0x00000000004ce7b5 in ngx_spawn_process (cycle=cycle@entry=0x34f8410, proc=0x4cffe0 <ngx_worker_process_cycle>, data=0x2,
    name=0xd9da9d "worker process", respawn=respawn@entry=2) at src/os/unix/ngx_process.c:199
#13 0x00000000004d13cd in ngx_reap_children (cycle=0x34f8410) at src/os/unix/ngx_process_cycle.c:622
#14 ngx_master_process_cycle (cycle=cycle@entry=0x34f8410) at src/os/unix/ngx_process_cycle.c:175
#15 0x00000000004a413a in main (argc=<optimized out>, argv=<optimized out>) at src/core/nginx.c:382
(gdb) p r
$1 = (ngx_http_request_t *) 0x864f640
(gdb) p r.cache
$2 = (ngx_http_cache_t *) 0x0

​It seems that r->cache can actually become NULL without r->cached reset to 0, so I believe it makes sense to check r->cache for NULL before dereferencing it.

See proposed patch attached.

[Roman Arutyunyan]

Thanks for reporting this. The issue is reproducible with error_page 412 and a conditional client request for a cached response.

http {
    proxy_cache_path cache keys_zone=foo:10m;

    server {
        listen 8000;
        location = / {
            proxy_pass http://127.0.0.1:8001;
            proxy_cache_valid any 1h;
            proxy_cache foo;
            error_page 412 /pf;
        }
        location = /pf {
            return 200 foo;
        }
    }

    server {
        listen 8001;
        location / {
            root html;
        }
    }
}

The second request leads to segfault:

$ curl -i 127.0.0.1:8000
$ curl -i -H 'If-Match: 1' 127.0.0.1:8000

I'm not sure however whether the proposed fix is good. The value of of r->cache is irrelevant in this case. And it could not be NULL if cache was configured for the error_page location. The same is true for r->cached.

[Maxim Dounin]

This looks like another issue with filter finalization. We've already have a bunch of these, see image_filter_finalize.t and not_modified_finalize.t tests. I don't think there is a good solution apart from ​moving r->cache to r->upstream.

[Roman Arutyunyan <arut@…>]

In 7514:319242d2ddc9/nginx:

Upstream: background cache update before cache send (ticket #1782).

In case of filter finalization, essential request fields like r->uri,
r->args etc could be changed, which affected the cache update subrequest.
Also, after filter finalization r->cache could be set to NULL, leading to
null pointer dereference in ngx_http_upstream_cache_background_update().
The fix is to create background cache update subrequest before sending the
cached response.

Since initial introduction in 1aeaae6e9446 (1.11.10) background cache update
subrequest was created after sending the cached response because otherwise it
blocked the parent request output. In 9552758a786e (1.13.1) background
subrequests were introduced to eliminate the delay before sending the final
part of the cached response. This also made it possible to create the
background cache update subrequest before sending the response.

Note that creating the subrequest earlier does not change the fact that in case
of filter finalization the background cache update subrequest will likely not
have enough time to successfully update the cache entry. Filter finalization
leads to the main request termination as soon the current iteration of request
processing is complete.

[Sergey Kandaurov <pluknet@…>]

In 1484:54b92955e4e2/nginx-tests:

Tests: not_modified_finalize.t with cached response (ticket #1782).