Patch for http proxying bug with some old gcc.

[Maxim Dounin]

Hello!

On Fri, May 13, 2016 at 01:04:40PM +0800, Vadim Panov wrote:

> Hi Guys,
> 
> Could you please review and consider including the attached patch. It fixes
> a bug which is only created by some gcc's, you will understand looking at
> it.
> 
> I reproduce using nginx with openwrt on marvell platform, so I can't tell
> what else might be affected.
> 
> How it works.
> upstream do something like this
> for(;;) {
> still some data
>    something->handle_header(some data)
> }
> and proxying module changes value of handle_header to some new function
> inside handle_header. if we have enough data to still stay in this loop
> some gcc would call a wrong pointer. (this bug is floating as if data goes
> in slow we exit this for loop)
> 
> Proposed patch only fixes consequences as it is not an upstream bug, it is
> a gcc bug. If there would be some trick adding volatile keyword the
> resulting code would be too fragile to stay cross compilable and cross
> platform. So checking one pointer seems a reasonable price to pay
> considering some gcc can do this with a loop and using this kind of loop to
> read remaining data is a very common pattern is the project design.
> 
> regards

> # HG changeset patch
> # User Vadim Panov <vadim.v.panov at gmail.com>
> # Date 1463114406 -28800
> #      Fri May 13 12:40:06 2016 +0800
> # Node ID 66f5e8d930f7b51dea90ebda5aa90dcf7fa6fff9
> # Parent  db699978a33fe19d786e95e00143eb2b6a1662c0
> fix http proxying on version compiled with some old gcc's
> 
> diff -r db699978a33f -r 66f5e8d930f7 src/http/modules/ngx_http_proxy_module.c
> --- a/src/http/modules/ngx_http_proxy_module.c	Thu May 12 16:43:19 2016 +0300
> +++ b/src/http/modules/ngx_http_proxy_module.c	Fri May 13 12:40:06 2016 +0800
> @@ -1686,6 +1686,15 @@
>  
>      u = r->upstream;
>  
> +    /* Pointer to this function seems get cached in a calling loop with
> +       some gcc versions. Even this function change the pointer in the end,
> +       the caller may still call this function instead of a new one */
> +    if (u->process_header != ngx_http_proxy_process_status_line) {
> +        ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
> +                       "http proxy: wrong cached u->process_header call");
> +        return u->process_header(r);
> +    }
> +
>      rc = ngx_http_parse_status_line(r, &u->buffer, &ctx->status);
>  
>      if (rc == NGX_AGAIN) {

It looks like you are talking about the problem as observed by 
some on Raspberry Pi with GCC 4.9, see

https://trac.nginx.org/nginx/ticket/748
https://trac.nginx.org/nginx/ticket/912

Given it's a compiler bug, and it's not eliminated by the change, 
but rather worked around in a way affecting other platforms as 
well, I don't think this patch should be considered.

Rather, I would recommend to avoid using -O2 on the affected 
platforms, but use -O1 as nginx does by default.  As per reports, it 
works just fine.  Alternatively, you can consider upgrading to a 
newer GCC version.

-- 
Maxim Dounin
http://nginx.org/