Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Aug 25, 2023 •
edited

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.

jay
jay reviewed Aug 27, 2023
View reviewed changes
lib/http2.c Outdated Show resolved Hide resolved
icing
icing requested changes Aug 28, 2023
View reviewed changes
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.

lib/http2.c Outdated Show resolved Hide resolved
@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
@vszakats @ptitSeb
lib: fix null ptr derefs and uninitialized vars (h2/h3)
fbd5a00 
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
Reviewers

@jay jay jay left review comments

@icing icing icing approved these changes

Assignees
No one assigned
Labels
HTTP/2 HTTP/3 h3 or quic related quiche Cloudflare's QUIC and HTTP/3 library
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@vszakats @jay @icing