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@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@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:

--- a/src/http/ngx_http_request.c
+++ b/src/http/ngx_http_request.c
@@ -714,6 +714,9 @@ ngx_http_ssl_handshake_handler(ngx_conne
static const ngx_str_t spdy = ngx_string(NGX_SPDY_NPN_NEGOTIATED);

len = 0;
+#if (NGX_SUPPRESS_WARN)
+ data = NULL;
+#endif

#ifdef TLSEXT_TYPE_application_layer_protocol_negotiation
SSL_get0_alpn_selected(c->ssl->connection, &data, &len);

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

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel