Skip to content

lib: fix null ptr derefs and uninitialized vars (h2/h3)#11739

Closed
vszakats wants to merge 3 commits intocurl:masterfrom
vszakats:warnfix
Closed

lib: fix null ptr derefs and uninitialized vars (h2/h3)#11739
vszakats wants to merge 3 commits intocurl:masterfrom
vszakats:warnfix

Conversation

@vszakats
Copy link
Member

@vszakats vszakats commented Aug 25, 2023

Closes #11739


FIXME, could not figure how to fix/silence this one [NOW FIXED]:

In function 'Curl_bufq_is_empty',
    inlined from 'stream_recv' at /test/curl/lib/http2.c:1824:7:
/test/curl/lib/bufq.c:308:12: warning: potential null pointer dereference [-Wnull-dereference]
  308 |   return !q->head || chunk_is_empty(q->head);
      |           ~^~~~~~
/test/curl/lib/bufq.c:308:12: warning: potential null pointer dereference [-Wnull-dereference]

Warnings from gcc 13.2.0, unity build:

In file included from /test/curl/bld-cmake-gcc-x64/lib/CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c:31:
/test/curl/lib/cf-h2-proxy.c: In function 'proxy_nw_in_reader':
/test/curl/lib/cf-h2-proxy.c:250:11: warning: null pointer dereference [-Wnull-dereference]
  250 |   nread = Curl_conn_cf_recv(cf->next, data, (char *)buf, buflen, err);
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/test/curl/lib/cf-h2-proxy.c: In function 'proxy_h2_nw_out_writer':
/test/curl/lib/cf-h2-proxy.c:264:14: warning: null pointer dereference [-Wnull-dereference]
  264 |   nwritten = Curl_conn_cf_send(cf->next, data, (const char *)buf, buflen, err);
      |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function 'proxy_nw_in_reader',
    inlined from 'chunk_slurpn' at /test/curl/lib/bufq.c:109:11,
    inlined from 'Curl_bufq_sipn' at /test/curl/lib/bufq.c:616:11,
    inlined from 'bufq_slurpn.constprop' at /test/curl/lib/bufq.c:645:9:
/test/curl/lib/cf-h2-proxy.c:250:11: warning: null pointer dereference [-Wnull-dereference]
  250 |   nread = Curl_conn_cf_recv(cf->next, data, (char *)buf, buflen, err);
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function 'Curl_bufq_is_empty',
    inlined from 'stream_recv' at /test/curl/lib/http2.c:1824:7:
/test/curl/lib/bufq.c:308:12: warning: potential null pointer dereference [-Wnull-dereference]
  308 |   return !q->head || chunk_is_empty(q->head);
      |           ~^~~~~~
/test/curl/lib/bufq.c:308:12: warning: potential null pointer dereference [-Wnull-dereference]
In file included from /test/curl/lib/sendf.h:29,
                 from /test/curl/lib/altsvc.c:37,
                 from /test/curl/bld-cmake-gcc-x64/lib/CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c:4:
/test/curl/lib/vquic/vquic.c: In function 'recvfrom_packets.constprop':
/test/curl/lib/curl_trc.h:83:15: warning: 'r_ip' may be used uninitialized [-Wmaybe-uninitialized]
   83 | #define failf Curl_failf
/test/curl/lib/vquic/vquic.c:471:9: note: in expansion of macro 'failf'
  471 |         failf(data, "QUIC: connection to %s port %u refused",
      |         ^~~~~
In file included from /test/curl/bld-cmake-gcc-x64/lib/CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c:484:
/test/curl/lib/vquic/vquic.c:467:21: note: 'r_ip' was declared here
  467 |         const char *r_ip;
      |                     ^~~~
/test/curl/lib/curl_trc.h:83:15: warning: 'r_port' may be used uninitialized [-Wmaybe-uninitialized]
   83 | #define failf Curl_failf
/test/curl/lib/vquic/vquic.c:471:9: note: in expansion of macro 'failf'
  471 |         failf(data, "QUIC: connection to %s port %u refused",
      |         ^~~~~
/test/curl/lib/vquic/vquic.c:468:13: note: 'r_port' was declared here
  468 |         int r_port;
      |             ^~~~~~
/test/curl/lib/vquic/vquic.c: In function 'recvfrom_packets':
/test/curl/lib/curl_trc.h:83:15: warning: 'r_ip' may be used uninitialized [-Wmaybe-uninitialized]
   83 | #define failf Curl_failf
/test/curl/lib/vquic/vquic.c:471:9: note: in expansion of macro 'failf'
  471 |         failf(data, "QUIC: connection to %s port %u refused",
      |         ^~~~~
/test/curl/lib/vquic/vquic.c:467:21: note: 'r_ip' was declared here
  467 |         const char *r_ip;
      |                     ^~~~
/test/curl/lib/curl_trc.h:83:15: warning: 'r_port' may be used uninitialized [-Wmaybe-uninitialized]
   83 | #define failf Curl_failf
/test/curl/lib/vquic/vquic.c:471:9: note: in expansion of macro 'failf'
  471 |         failf(data, "QUIC: connection to %s port %u refused",
      |         ^~~~~
/test/curl/lib/vquic/vquic.c:468:13: note: 'r_port' was declared here
  468 |         int r_port;
      |             ^~~~~~
In file included from /test/curl/bld-cmake-gcc-x64/lib/CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c:478:
In function 'cf_connect_start',
    inlined from 'cf_ngtcp2_connect' at /test/curl/lib/vquic/curl_ngtcp2.c:2468:14:
/test/curl/lib/vquic/curl_ngtcp2.c:2408:20: warning: 'sockaddr' may be used uninitialized [-Wmaybe-uninitialized]
 2408 |                    &sockaddr->sa_addr, sockaddr->addrlen);
/test/curl/lib/vquic/curl_ngtcp2.c: In function 'cf_ngtcp2_connect':
/test/curl/lib/vquic/curl_ngtcp2.c:2352:34: note: 'sockaddr' was declared here
 2352 |   const struct Curl_sockaddr_ex *sockaddr;
      |                                  ^~~~~~~~
/test/curl/lib/curl_trc.h:120:10: warning: 'r_ip' may be used uninitialized [-Wmaybe-uninitialized]
  120 |          Curl_infof(data, __VA_ARGS__); } while(0)
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/test/curl/lib/vquic/curl_ngtcp2.c:2533:5: note: in expansion of macro 'infof'
 2533 |     infof(data, "QUIC connect to %s port %u failed: %s",
      |     ^~~~~
/test/curl/lib/vquic/curl_ngtcp2.c:2528:17: note: 'r_ip' was declared here
 2528 |     const char *r_ip;
      |                 ^~~~
/test/curl/lib/curl_trc.h:120:10: warning: 'r_port' may be used uninitialized [-Wmaybe-uninitialized]
  120 |          Curl_infof(data, __VA_ARGS__); } while(0)
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/test/curl/lib/vquic/curl_ngtcp2.c:2533:5: note: in expansion of macro 'infof'
 2533 |     infof(data, "QUIC connect to %s port %u failed: %s",
      |     ^~~~~
/test/curl/lib/vquic/curl_ngtcp2.c:2529:9: note: 'r_port' was declared here
 2529 |     int r_port;
      |         ^~~~~~

@vszakats vszakats added HTTP/2 HTTP/3 h3 or quic related quiche Cloudflare's QUIC and HTTP/3 library labels Aug 25, 2023
@vszakats vszakats requested a review from icing August 25, 2023 22:35
@jay
Copy link
Member

jay commented Aug 26, 2023

In function 'Curl_bufq_is_empty',
inlined from 'stream_recv' at /test/curl/lib/http2.c:1824:7:

/test/curl/lib/bufq.c:308:12: warning: potential null pointer dereference [-Wnull-dereference]
  308 |   return !q->head || chunk_is_empty(q->head);
      |           ~^~~~~~
/test/curl/lib/bufq.c:308:12: warning: potential null pointer dereference [-Wnull-dereference]

curl/lib/http2.c

Lines 1816 to 1826 in c2212c0

static ssize_t stream_recv(struct Curl_cfilter *cf, struct Curl_easy *data,
char *buf, size_t len, CURLcode *err)
{
struct cf_h2_ctx *ctx = cf->ctx;
struct stream_ctx *stream = H2_STREAM_CTX(data);
ssize_t nread = -1;
*err = CURLE_AGAIN;
if(!Curl_bufq_is_empty(&stream->recvbuf)) {
nread = Curl_bufq_read(&stream->recvbuf,
(unsigned char *)buf, len, err);

If it's inlining then maybe it means stream may be null which can happen at other points. For example

curl/lib/http2.c

Lines 2493 to 2502 in c2212c0

static bool cf_h2_data_pending(struct Curl_cfilter *cf,
const struct Curl_easy *data)
{
struct cf_h2_ctx *ctx = cf->ctx;
struct stream_ctx *stream = H2_STREAM_CTX(data);
if(ctx && (!Curl_bufq_is_empty(&ctx->inbufq)
|| (stream && !Curl_bufq_is_empty(&stream->sendbuf))
|| (stream && !Curl_bufq_is_empty(&stream->recvbuf))))
return TRUE;

From what I can see stream can be null if http2_data_setup hasn't been called which means nothing has been sent. A quick fix would be stream && for everywhere it's derefed in that function. A better fix would be to initialize the stream context earlier, but that may have side effects.

@vszakats
Copy link
Member Author

@jay: Thanks for looking into this. Based on it, I could fix this in http2.c by adding a single if(!stream).

It might use a more complete fix to not just silence the warning, but tackle other potential cases. Also, the returned failure value might not be the one we want and/or could also use trace output.

Copy link
Contributor

@icing icing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the proposed parameter addition might make things more clear and avoid unnecessary ifs for cases that never happen.

@vszakats
Copy link
Member Author

Pushed a commit to pass stream as a parameter.

@vszakats vszakats closed this in d50fe6b Aug 28, 2023
@vszakats vszakats deleted the warnfix branch August 28, 2023 19:48
ptitSeb pushed a commit to wasix-org/curl that referenced this pull request Sep 25, 2023
Fixing compiler warnings with gcc 13.2.0 in unity builds.

Assisted-by: Jay Satiro
Assisted-by: Stefan Eissing
Closes curl#11739
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

HTTP/2 HTTP/3 h3 or quic related quiche Cloudflare's QUIC and HTTP/3 library

Development

Successfully merging this pull request may close these issues.

3 participants