Skip to content

Fix leak in bssl-compat SSL_get_servername()#427

Merged
tedjpoole merged 1 commit intoenvoyproxy:release/v1.35from
tedjpoole:fix_ssl_get_servername_leak_1_35
Dec 4, 2025
Merged

Fix leak in bssl-compat SSL_get_servername()#427
tedjpoole merged 1 commit intoenvoyproxy:release/v1.35from
tedjpoole:fix_ssl_get_servername_leak_1_35

Conversation

@tedjpoole
Copy link
Copy Markdown
Contributor

Fixes a leak that can occur if multiple calls are made to SSL_get_servername() from within the same certificate selection callback (see SSL_CTX_set_select_certificate_cb()).

Signed-off-by: Ted Poole <tpoole@redhat.com>
@tedjpoole tedjpoole requested a review from twghu December 4, 2025 09:45
@twghu
Copy link
Copy Markdown
Contributor

twghu commented Dec 4, 2025

The fix looks good :)

I understand the test was run under valgrind.

This might be a candidate pattern for future memory leak tests:

  TEST(SSLTest, test_SSL_get_servername_no_leak) {
    // Track memory before
    size_t mem_before = GetCurrentMemoryUsage();

    // Exercise the code
    for (int i = 0; i < 1000; i++) {
      SSL_get_servername(...);
    }

    // Force cleanup
    SSL_free(ssl);

    // Verify memory returned
    size_t mem_after = GetCurrentMemoryUsage();
    EXPECT_NEAR(mem_before, mem_after, tolerance);
  }

Copy link
Copy Markdown
Contributor

@twghu twghu left a comment

Choose a reason for hiding this comment

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

lgtm

@tedjpoole tedjpoole merged commit 2ef3e46 into envoyproxy:release/v1.35 Dec 4, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants