ngx_http_copy_pipelined_header didn't adjust buffer pointer after copy data

[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	}

[Maxim Dounin]

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.

[zhuizhuhaomeng@…]

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?

[Maxim Dounin]

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.

[zhuizhuhaomeng@…]

Thank you