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.

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

Socket-leak in 'vquic/msh3.c' #8915

Closed
gvanem opened this issue May 25, 2022 · 8 comments
Closed

Socket-leak in 'vquic/msh3.c' #8915

gvanem opened this issue May 25, 2022 · 8 comments
Labels
HTTP/3 h3 or quic related memory-leak

Comments

@gvanem
Copy link
Contributor

gvanem commented May 25, 2022

Building with most options enabled and doing:

set CURL_MEMDEBUG=c:\temp\memdebug.curl
curl --http3 https://www.curl.se
perl tests\memanalyze.pl c:\temp\memdebug.curl

gives: Open file descriptor created at connect.c:1632

@bagder bagder added memory-leak HTTP/3 h3 or quic related labels May 25, 2022
@bagder
Copy link
Member

bagder commented Aug 11, 2022

@nibanks when I just now reproduced this issue with valgrind on my curl+msh3 build, it certainly shows leaking memory.

But the more primary problem I fell over is that I get a lot of "Conditional jump or move depends on uninitialised value(s)" errors from valgrind that overshadow the leak and that make it hard to work on the leak. I don't know yet if the leak is because of a curl bug or something msh3 should do. This test ran on a Debian Linux x86_64.

Here is my valgrind output from a curl build on current git master: error.txt curl's output is here:
curl-out.txt (I used an msh3 build freshly built from git master just now, git describe --tags calls it v0.3.0-17-gfa550fd).

I used this command line:

valgrind --log-file=error ./src/curl --http3 https://curl.se -v

@nibanks
Copy link
Contributor

nibanks commented Aug 11, 2022

I'll take a look!

@nibanks
Copy link
Contributor

nibanks commented Aug 11, 2022

I think the following would fix the EncodeHeaders issue:

--- a/lib/msh3.hpp
+++ b/lib/msh3.hpp
@@ -52,6 +52,7 @@ enum H3SettingsType {
 // Contiguous buffer for (non-null-terminated) header name and value strings.
 struct H3HeadingPair : public lsxpack_header_t {
     char Buffer[512] = {0};
+    H3HeadingPair() { memset(this, 0, sizeof(lsxpack_header_t)); }
     bool Set(const MSH3_HEADER* Header) {
         if (Header->NameLength + Header->ValueLength > sizeof(Buffer)) return false;
         buf = Buffer;
diff --git a/msquic b/msquic
index 030273f..3a46222 160000
--- a/msquic
+++ b/msquic

If you're able to add the new line to msh3.hpp to try while I look at the other error, I'd appreciate it.

@nibanks
Copy link
Contributor

nibanks commented Aug 11, 2022

For this one, I'm not sure why it claims there is uninitialized memory access.

==1929954== Syscall param sendmsg(mmsg[0].msg_hdr.msg_iov[0]) points to uninitialised byte(s)
==1929954==    at 0x4E40936: sendmmsg (sendmmsg.c:31)
==1929954==    by 0x4FAD649: CxPlatSocketSendInternal (in /home/daniel/build-msh3/lib/libmsquic.so.2.0.0)
==1929954==    by 0x4FADC48: CxPlatSocketSend (in /home/daniel/build-msh3/lib/libmsquic.so.2.0.0)
==1929954==    by 0x4F85AFC: QuicBindingSend (in /home/daniel/build-msh3/lib/libmsquic.so.2.0.0)
==1929954==    by 0x4FA6B15: QuicPacketBuilderSendBatch (in /home/daniel/build-msh3/lib/libmsquic.so.2.0.0)
==1929954==    by 0x4FA6C1D: QuicPacketBuilderFinalize (in /home/daniel/build-msh3/lib/libmsquic.so.2.0.0)
==1929954==    by 0x4FA2181: QuicSendFlush (in /home/daniel/build-msh3/lib/libmsquic.so.2.0.0)
==1929954==    by 0x4F8F773: QuicConnDrainOperations (in /home/daniel/build-msh3/lib/libmsquic.so.2.0.0)
==1929954==    by 0x4F80A30: QuicWorkerProcessConnection (in /home/daniel/build-msh3/lib/libmsquic.so.2.0.0)
==1929954==    by 0x4F80DF6: QuicWorkerLoop (in /home/daniel/build-msh3/lib/libmsquic.so.2.0.0)
==1929954==    by 0x4F80FCF: QuicWorkerThread (in /home/daniel/build-msh3/lib/libmsquic.so.2.0.0)
==1929954==    by 0x4DBCB26: start_thread (pthread_create.c:435)

My first thought it that we're only partially initializing the memory that we actually want to send out, and then pass the length appropriate with what has been initialized. Would valgrind still complain about that?

@bagder
Copy link
Member

bagder commented Aug 11, 2022

@nibanks once I bumped my build to nibanks/msh3@e30c9ce I don't see any more uninitialized warnings. Do you still see any issue?

I still see a memory leak, but I'll work on that.

@bagder
Copy link
Member

bagder commented Aug 12, 2022

@nibanks: question: the memory allocated here:

stream->req = MsH3RequestOpen(qs->conn, &msh3_request_if, stream,

... where is that freed again? Is that part of the connection so it gets freed with MsH3ConnectionClose() ?

The reason I ask is that I see an occasional memory leak that valgrind reports like this (line numbers from my slightly edited dev version):

==2178340== Memcheck, a memory error detector
==2178340== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==2178340== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==2178340== Command: ./src/curl --http3 https://curl.se -v -o h3
==2178340== Parent PID: 1415441
==2178340== 
==2178340== 
==2178340== HEAP SUMMARY:
==2178340==     in use at exit: 23,104 bytes in 12 blocks
==2178340==   total heap usage: 8,036 allocs, 8,024 frees, 1,637,203 bytes allocated
==2178340== 
==2178340== 22,856 (1,544 direct, 21,312 indirect) bytes in 1 blocks are definitely lost in loss record 12 of 12
==2178340==    at 0x48417B5: operator new(unsigned long, std::nothrow_t const&) (vg_replace_malloc.c:530)
==2178340==    by 0x4856EC1: MsH3Connection::SendRequest(MSH3_REQUEST_IF const*, void*, MSH3_HEADER const*, unsigned long) (in /home/daniel/build-msh3/lib/libmsh3.so)
==2178340==    by 0x1838FB: msh3_stream_send (msh3.c:412)
==2178340==    by 0x15C300: Curl_write (sendf.c:318)
==2178340==    by 0x1A4257: Curl_buffer_send (http.c:1339)
==2178340==    by 0x1A6EB3: Curl_http_bodysend (http.c:2701)
==2178340==    by 0x1A85E2: Curl_http (http.c:3271)
==2178340==    by 0x183100: msh3_do_it (msh3.c:217)
==2178340==    by 0x1543D6: multi_do (multi.c:1563)
==2178340==    by 0x15559E: multi_runsingle (multi.c:2112)
==2178340==    by 0x15683A: curl_multi_perform (multi.c:2640)
==2178340==    by 0x14139C: easy_transfer (easy.c:662)
==2178340== 
==2178340== LEAK SUMMARY:
==2178340==    definitely lost: 1,544 bytes in 1 blocks
==2178340==    indirectly lost: 21,312 bytes in 5 blocks
==2178340==      possibly lost: 0 bytes in 0 blocks
==2178340==    still reachable: 248 bytes in 6 blocks
==2178340==                       of which reachable via heuristic:
==2178340==                         length64           : 248 bytes in 6 blocks
==2178340==         suppressed: 0 bytes in 0 blocks
==2178340== Reachable blocks (those to which a pointer was found) are not shown.
==2178340== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==2178340== 
==2178340== For lists of detected and suppressed errors, rerun with: -s
==2178340== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

@nibanks
Copy link
Contributor

nibanks commented Aug 12, 2022

It looks like we're missing a call to MsH3RequestClose for the stream->req. Not sure how I missed that originally. Sorry.

You just need to call that when you're done with the request. Though, I'm not 100% positive when that is in your interface.

@bagder
Copy link
Member

bagder commented Aug 12, 2022

Ok, thanks. I'm on it.

bagder added a commit that referenced this issue Aug 12, 2022
And free request related memory better in 'done'. Fixes a memory-leak.

Reported-by: Gisle Vanem
Fixes #8915
bagder added a commit that referenced this issue Aug 12, 2022
And free request related memory better in 'done'. Fixes a memory-leak.

Reported-by: Gisle Vanem
Fixes #8915
@bagder bagder closed this as completed in 011788f Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HTTP/3 h3 or quic related memory-leak
Development

No branches or pull requests

3 participants