Opened 5 weeks ago

Closed 5 weeks ago

Last modified 5 weeks ago

#2068 closed defect (invalid)

ngx_http_copy_pipelined_header didn't adjust buffer pointer after copy data

Reported by: zhuizhuhaomeng@… Owned by:
Priority: major Milestone: nginx-1.19
Component: nginx-core Version: 1.19.x
Keywords: Cc:
uname -a: Linux localhost.localdomain 4.18.0-193.6.3.el8_2.x86_64 #1 SMP Wed Jun 10 11:09:32 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
nginx -V: nginx version: openresty/1.19.3.1rc0
built by gcc 8.3.1 20191121 (Red Hat 8.3.1-5) (GCC)
built with OpenSSL 1.1.1g 21 Apr 2020
TLS SNI support enabled
configure arguments: --prefix=/usr/local/openresty/nginx --with-debug --with-cc-opt='-DNGX_LUA_USE_ASSERT -DNGX_LUA_ABORT_AT_PANIC -O2 -DNGX_LUA_ABORT_AT_PANIC -I/usr/local/openresty/zlib/include -I/usr/local/openresty/pcre/include -I/usr/local/openresty/openssl111/include' --add-module=../ngx_devel_kit-0.3.1 --add-module=../echo-nginx-module-0.62 --add-module=../xss-nginx-module-0.06 --add-module=../ngx_coolkit-0.2 --add-module=../set-misc-nginx-module-0.32 --add-module=../form-input-nginx-module-0.12 --add-module=../encrypted-session-nginx-module-0.08 --add-module=../srcache-nginx-module-0.32 --add-module=../ngx_lua-0.10.18rc3 --add-module=../ngx_lua_upstream-0.07 --add-module=../headers-more-nginx-module-0.33 --add-module=../array-var-nginx-module-0.05 --add-module=../memc-nginx-module-0.19 --add-module=../redis2-nginx-module-0.15 --add-module=../redis-nginx-module-0.3.7 --add-module=../ngx_stream_lua-0.0.9rc3 --with-ld-opt='-Wl,-rpath,/usr/local/openresty/luajit/lib -L/usr/local/openresty/zlib/lib -L/usr/local/openresty/pcre/lib -L/usr/local/openresty/openssl111/lib -Wl,-rpath,/usr/local/openresty/zlib/lib:/usr/local/openresty/pcre/lib:/usr/local/openresty/openssl111/lib' --with-cc='ccache gcc -fdiagnostics-color=always' --with-pcre-jit --with-stream --with-stream_ssl_module --with-stream_ssl_preread_module --with-http_v2_module --without-mail_pop3_module --without-mail_imap_module --without-mail_smtp_module --with-http_stub_status_module --with-http_realip_module --with-http_addition_module --with-http_auth_request_module --with-http_secure_link_module --with-http_random_index_module --with-http_gzip_static_module --with-http_sub_module --with-http_dav_module --with-http_flv_module --with-http_mp4_module --with-http_gunzip_module --with-threads --with-compat --with-stream --with-http_ssl_module

Description (last modified by zhuizhuhaomeng@…)

we have found a bug with our test suite with our ngx module.

when HTTP pipelined request arrived, the header of the next request will be copied to r->header_in, but the original request buffer was not adjust.
I think it is better to adjust the original request body buffer last pointer after copy the data to new buffer.

The patch is bellow:

diff --git a/src/http/ngx_http_request_body.c b/src/http/ngx_http_request_body.c
index 71d7e9ab..fe075a87 100644
--- a/src/http/ngx_http_request_body.c
+++ b/src/http/ngx_http_request_body.c
@@ -472,6 +472,7 @@ ngx_http_copy_pipelined_header(ngx_http_request_t *r, ngx_buf_t *buf)
     }
 
     ngx_memcpy(b->last, buf->pos, n);
+    buf->last -= n;
 
     b->last += n;
     r->request_length -= n;

   401	static ngx_int_t
   402	ngx_http_copy_pipelined_header(ngx_http_request_t *r, ngx_buf_t *buf)
   403	{
   404	    size_t                     n;
   405	    ngx_buf_t                 *b;
   406	    ngx_chain_t               *cl;
   407	    ngx_http_connection_t     *hc;
   408	    ngx_http_core_srv_conf_t  *cscf;
   409	
   410	    b = r->header_in;
   411	    n = buf->last - buf->pos;
   412	
   413	    if (buf == b || n == 0) {
   414	        return NGX_OK;
   415	    }
   416	
   417	    ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
   418	                   "http body pipelined header: %uz", n);
   419	
   420	    /*
   421	     * if there is a pipelined request in the client body buffer,
   422	     * copy it to the r->header_in buffer if there is enough room,
   423	     * or allocate a large client header buffer
   424	     */
   425	
   426	    if (n > (size_t) (b->end - b->last)) {
   427	
   428	        hc = r->http_connection;
   429	
   430	        if (hc->free) {
   431	            cl = hc->free;
   432	            hc->free = cl->next;
   433	
   434	            b = cl->buf;
   435	
   436	            ngx_log_debug2(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
   437	                           "http large header free: %p %uz",
   438	                           b->pos, b->end - b->last);
   439	
   440	        } else {
   441	            cscf = ngx_http_get_module_srv_conf(r, ngx_http_core_module);
   442	
   443	            b = ngx_create_temp_buf(r->connection->pool,
   444	                                    cscf->large_client_header_buffers.size);
   445	            if (b == NULL) {
   446	                return NGX_ERROR;
   447	            }
   448	
   449	            cl = ngx_alloc_chain_link(r->connection->pool);
   450	            if (cl == NULL) {
   451	                return NGX_ERROR;
   452	            }
   453	
   454	            cl->buf = b;
   455	
   456	            ngx_log_debug2(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
   457	                           "http large header alloc: %p %uz",
   458	                           b->pos, b->end - b->last);
   459	        }
   460	
   461	        cl->next = hc->busy;
   462	        hc->busy = cl;
   463	        hc->nbusy++;
   464	
   465	        r->header_in = b;
   466	
   467	        if (n > (size_t) (b->end - b->last)) {
   468	            ngx_log_error(NGX_LOG_ALERT, r->connection->log, 0,
   469	                          "too large pipelined header after reading body");
   470	            return NGX_ERROR;
   471	        }
   472	    }
   473	
   474	    ngx_memcpy(b->last, buf->pos, n);
   475	    buf->last -= n;  //here is the modification
   476	
   477	    b->last += n;
   478	    r->request_length -= n;
   479	
   480	    return NGX_OK;
   481	}

Change History (6)

comment:1 by zhuizhuhaomeng@…, 5 weeks ago

Description: modified (diff)

comment:2 by Maxim Dounin, 5 weeks ago

Please clarify why you think this is a bug in nginx. The request body buffer's last pointer is not expected to be used after the ngx_http_copy_pipelined_header() call. That is, adjusting the buf->last would be a dead store.

If your module tries to do something with r->request_body->buf, I would rather suggest this is a bug in your module.

comment:3 by zhuizhuhaomeng@…, 5 weeks ago

In our module, we calculate request body length with
r->request_body->buf->last - r->request_body->buf->pos;

As your suggestion, we should not calculate the body length this way.
Maybe use r->headers_in.content_length_n directly?

Last edited 5 weeks ago by zhuizhuhaomeng@… (previous) (diff)

comment:4 by zhuizhuhaomeng@…, 5 weeks ago

Description: modified (diff)

in reply to:  3 comment:5 by Maxim Dounin, 5 weeks ago

Resolution: invalid
Status: newclosed

Replying to zhuizhuhaomeng@…:

In our module, we calculate request body length with
r->request_body->buf->last - r->request_body->buf->pos;

This is certainly incorrect and not going to work.

As your suggestion, we should not calculate the body length this way.
Maybe use r->headers_in.content_length_n directly?

The r->headers_in.content_length_n is a good choice, though note that it might not be available if the request body isn't fully read.

Also, a good approach might be to count total size of the buffers in r->requst_body->bufs, as in the example code in the Developmet guide.

comment:6 by zhuizhuhaomeng@…, 5 weeks ago

Thank you

Note: See TracTickets for help on using tickets.