[nginx] SSL: support ALPN (IETF's successor to NPN).

[Maxim Dounin]

Hello!

On Thu, Jan 30, 2014 at 05:06:02PM +0400, Valentin V. Bartenev wrote:

> On Thursday 30 January 2014 16:45:18 Maxim Dounin wrote:
> > Hello!
> > 
> > On Thu, Jan 30, 2014 at 01:55:23PM +0400, Ruslan Ermilov wrote:
> > 
> > > On Wed, Jan 29, 2014 at 03:51:50PM +0000, Valentin Bartenev wrote:
> > > > details:   http://hg.nginx.org/nginx/rev/01e2a5bcdd8f
> > > > branches:  
> > > > changeset: 5545:01e2a5bcdd8f
> > > > user:      Piotr Sikora <piotr at cloudflare.com>
> > > > date:      Tue Jan 28 15:33:49 2014 -0800
> > > > description:
> > > > SSL: support ALPN (IETF's successor to NPN).
> > > > 
> > > > Signed-off-by: Piotr Sikora <piotr at cloudflare.com>
> > > 
> > > This change breaks compilation with clang on systems
> > > where OpenSSL doesn't have ALPN support:
> > > 
> > > : cc -c -pipe  -O -Wall -Wextra -Wpointer-arith -Wconditional-uninitialized -Wno-unused-parameter -Werror -g  -I src/core -I src/event -I src/event/modules -I src/os/unix -I /opt/local/include -I objs -I src/http -I src/http/modules \
> > > :                 -o objs/src/http/ngx_http_request.o \
> > > :                 src/http/ngx_http_request.c
> > > : src/http/ngx_http_request.c:728:44: error: variable 'data' may be uninitialized when used here [-Werror,-Wconditional-uninitialized]
> > > :         if (len == spdy.len && ngx_strncmp(data, spdy.data, spdy.len) == 0) {
> > > :                                            ^~~~
> > > : src/core/ngx_string.h:53:56: note: expanded from macro 'ngx_strncmp'
> > > : #define ngx_strncmp(s1, s2, n)  strncmp((const char *) s1, (const char *) s2, n)
> > > :                                                        ^
> > > : src/http/ngx_http_request.c:713:38: note: initialize the variable 'data' to silence this warning
> > > :         const unsigned char     *data;
> > > :                                      ^
> > > :                                       = NULL
> > > : 1 error generated.
> > > : make[1]: *** [objs/src/http/ngx_http_request.o] Error 1
> > > : make: *** [build] Error 2
> > > 
> > > clang(1) mistakenly thinks that "len" can be non-zero, and under
> > > this condition "data" may be uninitialized.
> > > 
> > > It's possible to rewrite the code such as that it doesn't trigger
> > > the warning:
> > > 
> > > diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c
> > > --- a/src/http/ngx_http_request.c
> > > +++ b/src/http/ngx_http_request.c
> > > @@ -713,11 +713,8 @@ ngx_http_ssl_handshake_handler(ngx_conne
> > >          const unsigned char     *data;
> > >          static const ngx_str_t   spdy = ngx_string(NGX_SPDY_NPN_NEGOTIATED);
> > >  
> > > -        len = 0;
> > > -
> > >  #ifdef TLSEXT_TYPE_application_layer_protocol_negotiation
> > >          SSL_get0_alpn_selected(c->ssl->connection, &data, &len);
> > > -#endif
> > >  
> > >  #ifdef TLSEXT_TYPE_next_proto_neg
> > >          if (len == 0) {
> > > @@ -725,6 +722,10 @@ ngx_http_ssl_handshake_handler(ngx_conne
> > >          }
> > >  #endif
> > >  
> > > +#else /* TLSEXT_TYPE_next_proto_neg */
> > > +        SSL_get0_next_proto_negotiated(c->ssl->connection, &data, &len);
> > > +#endif
> > > +
> > >          if (len == spdy.len && ngx_strncmp(data, spdy.data, spdy.len) == 0) {
> > >              ngx_http_spdy_init(c->read);
> > >              return;
> > 
> > That's clearly a false positive warning, and I would rather add "data = NULL"
> > under NGX_SUPPRESS_WARN as we usually do, something like this:
> [..]
> 
> Of course you have the final word, but I like Ruslan's solution.

Looks like I failed to convince Ruslan that my variant is better. 
:)

If you both prefer this variant I don't object.  Ruslan, please 
commit.

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