Skip to content

OpenSSL-QUIC: memory leak when downgrading from HTTP/3 #14720

Closed

Description

@ralfjunker

I did this

This minimum code generates a memory leak:

curl_easy_setopt(easy_handle, CURLOPT_SSL_VERIFYPEER, 0);
curl_easy_setopt(easy_handle, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_3);
curl_easy_setopt(easy_handle, CURLOPT_URL, "https://www.strato.de/");
curl_easy_perform(easy_handle);

It is important that the server does not run HTTP/3 to force curl to downgrade. Without the downgrade (that said, no CURL_HTTP_VERSION_3), there is no leak.

The leak was introduced in cb17c06.

This patch fixes the leak, but I did not otherwise test for correctness or unwanted side effects.

 lib/vquic/curl_osslq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/vquic/curl_osslq.c b/lib/vquic/curl_osslq.c
index 31469c8d43..229512a7f5 100644
--- a/lib/vquic/curl_osslq.c
+++ b/lib/vquic/curl_osslq.c
@@ -434,6 +434,7 @@ static void cf_osslq_destroy(struct Curl_cfilter *cf, struct Curl_easy *data)
   CURL_TRC_CF(data, cf, "destroy");
   if(ctx) {
     CURL_TRC_CF(data, cf, "cf_osslq_destroy()");
+    cf_osslq_ctx_close(ctx);
     cf_osslq_ctx_free(ctx);
   }
   cf->ctx = NULL;

I expected the following

No memory leak.

curl/libcurl version

GIT master as of today, Aug 28, 2024.

operating system

Windows 11 and OpenSSL 3.3.1.

Activity

  1. changed the title [-]Memory leak when downgrading from HTTP/3[/-] [+]OpenSSL-QUIC: memory leak when downgrading from HTTP/3[/+] on Aug 29, 2024
  2. added a commit that references this issue on Aug 29, 2024
  3. icing commented on Aug 29, 2024

    @icing
    Contributor

    Thanks for the nice report and the fix. I have applied it in #14724.

  4. ralfjunker commented on Aug 29, 2024

    @ralfjunker
    Author

    Thanks for the fast feedback!

    Unfortunately, I just noticed that my initial fix caused an AV with a HTTP/3 connection. This one fixes the downgrade leak without the AV. Maybe it is a better option?

     lib/connect.c | 1 +
     1 file changed, 1 insertion(+)
    
    diff --git a/lib/connect.c b/lib/connect.c
    index 9b68f15da5..13dad4c57a 100644
    --- a/lib/connect.c
    +++ b/lib/connect.c
    @@ -490,6 +490,7 @@ static void baller_close(struct eyeballer *baller,
                               struct Curl_easy *data)
     {
       if(baller && baller->cf) {
    +    baller->cf->cft->do_close(baller->cf, data);
         Curl_conn_cf_discard_chain(&baller->cf, data);
       }
     }
  5. icing commented on Aug 29, 2024

    @icing
    Contributor

    I added to the PR that the close only happens when the SSL* has been initialized. I think it is safer that "destroy" of the filter always cleans up, so callers should not worry.

  6. ralfjunker commented on Aug 29, 2024

    @ralfjunker
    Author

    For the record: Your additional cleanup conditional in a2baa2b also fixes the AV.

  7. added a commit that references this issue on Aug 29, 2024
    4abf2b9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions