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

warning: -Wnull-dereference with ngtcp2 + gcc 13.1.0 #11147

Closed
vszakats opened this issue May 18, 2023 · 4 comments
Closed

warning: -Wnull-dereference with ngtcp2 + gcc 13.1.0 #11147

vszakats opened this issue May 18, 2023 · 4 comments
Assignees
@icing
Labels
build

Comments

@vszakats
Copy link
Member

I did this

Built curl with gcc 13.1.0 and got this warning:

cd /my/curl/bld-cmake-gcc-x64-shared/lib && /usr/local/bin/x86_64-w64-mingw32-gcc -DBUILDING_LIBCURL -DCURL_HIDDEN_SYMBOLS -DHAVE_CONFIG_H -DUNICODE -D_UNICODE -Dlibcurl_EXPORTS -I/my/curl/include -I/my/brotli/x64-ucrt/usr/include -I/my/zstd/x64-ucrt/usr/include -I/my/nghttp2/x64-ucrt/usr/include -I/my/ngtcp2/x64-ucrt/usr/include -I/my/nghttp3/x64-ucrt/usr/include -I/my/libssh2/x64-ucrt/usr/include -I/my/curl/bld-cmake-gcc-x64-shared/lib/../include -I/my/curl/lib/.. -I/my/curl/lib/../include -I/my/curl/bld-cmake-gcc-x64-shared/lib/.. -I/my/curl/lib -I/my/curl/bld-cmake-gcc-x64-shared/lib -isystem /my/quictls/x64-ucrt/usr/include -isystem /my/zlib/x64-ucrt/usr/include -m64  -fno-ident  -D_UCRT  -DHAVE_SSL_SET0_WBIO -DHAS_ALPN -DNGHTTP2_STATICLIB -DNGHTTP3_STATICLIB -DNGTCP2_STATICLIB -DUSE_MANUAL=1 -m64  -specs=/my/gcc-specs-ucrt -static-libgcc  -lucrt  -W -Wall -pedantic -Wbad-function-cast -Wconversion -Winline -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-long-long -Wno-multichar -Wpointer-arith -Wshadow -Wsign-compare -Wundef -Wunused -Wwrite-strings -Wcast-align -Wdeclaration-after-statement -Wempty-body -Wendif-labels -Wfloat-equal -Wignored-qualifiers -Wno-format-nonliteral -Wno-sign-conversion -Wno-system-headers -Wstrict-prototypes -Wtype-limits -Wvla -Wmissing-parameter-type -Wold-style-declaration -Wstrict-aliasing=3 -Wno-pedantic-ms-format -Wformat=2 -Warray-bounds=2 -ftree-vrp -Wduplicated-cond -Wnull-dereference -fdelete-null-pointer-checks -Wshift-negative-value -Wshift-overflow=2 -Walloc-zero -Wduplicated-branches -Wformat-overflow=2 -Wformat-truncation=1 -Wrestrict -Warith-conversion -Wdouble-promotion -Wenum-conversion -Wunused-const-variable -O3 -DNDEBUG -fvisibility=hidden -municode -MD -MT lib/CMakeFiles/libcurl.dir/vquic/curl_ngtcp2.c.obj -MF CMakeFiles/libcurl.dir/vquic/curl_ngtcp2.c.obj.d -o CMakeFiles/libcurl.dir/vquic/curl_ngtcp2.c.obj -c /my/curl/lib/vquic/curl_ngtcp2.c
In function 'recv_closed_stream',
    inlined from 'cf_ngtcp2_recv' at /my/curl/lib/vquic/curl_ngtcp2.c:1416:15:
/my/curl/lib/vquic/curl_ngtcp2.c:1322:12: warning: potential null pointer dereference [-Wnull-dereference]
 1322 |   if(stream->reset) {
      |      ~~~~~~^~~~~~~
/my/curl/lib/vquic/curl_ngtcp2.c:1322:12: warning: potential null pointer dereference [-Wnull-dereference]

I expected the following

No warning.

curl/libcurl version

master @ 446061e

operating system

Cross-building for Windows.
@vszakats vszakats added the build label May 18, 2023
@vszakats vszakats changed the title ngtcp2 + gcc 13.1.0 -Wnull-dereference warning warning:-Wnull-dereference with ngtcp2 + gcc 13.1.0 May 18, 2023
@vszakats vszakats changed the title warning:-Wnull-dereference with ngtcp2 + gcc 13.1.0 warning: -Wnull-dereference with ngtcp2 + gcc 13.1.0 May 18, 2023
@bagder
Copy link
Member

bagder commented May 18, 2023

This looks similar to warnings you easily get with gcc's -fanalyzer option: I think this one happens because on line 1317:

  struct stream_ctx *stream = H3_STREAM_CTX(data);

... and that macro checks for data being NULL and if so it will return NULL and then there would be a NULL dereference. Of course that will only happen if data is NULL when this function is called.

There's also an assert there that would trigger if stream ever was NULL there in a debug build.

It's not immediately obvious to me how to silence this warning.

@bagder
Copy link
Member

bagder commented May 18, 2023

@icing can you give a shot?

@vszakats
Copy link
Member Author

vszakats commented May 18, 2023 •
edited

It slips through CI because all ngtcp2 builds have debug enabled, and in those cases the assert catches it. This silences the warning for non-debug builds:

--- a/lib/vquic/curl_ngtcp2.c
+++ b/lib/vquic/curl_ngtcp2.c
@@ -1319,6 +1319,8 @@ static ssize_t recv_closed_stream(struct Curl_cfilter *cf,
 
   (void)cf;
   DEBUGASSERT(stream);
+  if(!stream)
+    goto out;
   if(stream->reset) {
     failf(data,
           "HTTP/3 stream %" PRId64 " reset by server", stream->id);

icing added a commit to icing/curl that referenced this issue May 19, 2023
@icing
Fixing compiler warning about possible null-deref
0129cd8 
- compiler analyzer did not include the call context for this
  static function where the condition had already been checked.
- eleminating the problem by making stream a call parameter
- refs curl#11147
@icing icing mentioned this issue May 19, 2023
Closed
@icing
Copy link
Contributor

icing commented May 19, 2023

Pushed #11151 as fix for this. The warning case never happend as this static function was only called in a context where stream had already been checked. Fixing this by making stream a parameter.

@bagder bagder closed this as completed in e0ddfc8 May 19, 2023
bch pushed a commit to bch/curl that referenced this issue Jul 19, 2023
@icing @bch
ngtcp2: fix compiler warning about possible null-deref
ba55668 
- compiler analyzer did not include the call context for this
  static function where the condition had already been checked.
- eleminating the problem by making stream a call parameter

Fixes curl#11147
Closes curl#11151
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@icing icing

Labels
build
Milestone
No milestone
Development

No branches or pull requests
3 participants
@icing @bagder @vszakats