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

fix(us): memory leak when getting root certificate #16073

Merged
merged 1 commit into from
Dec 31, 2024

Conversation

DonIsaac
Copy link
Contributor

What does this PR do?

Fixes a memory leak when an x509 root certificate is successfully read for an https server.

How did you verify your code works?

There are already tests for this. I just ran this command to find+fix it:

leaks -atExit -- ./build/debug/bun-debug test ./test/js/bun/http/serve-body-leak.test.ts | tee leaks.log

There's a bunch of other reported leaks that I'll address in other PRs.

@robobun
Copy link

robobun commented Dec 31, 2024

Updated 6:53 PM PT - Dec 30th, 2024

@DonIsaac, your commit dd4b248 has 1 failures in #8699:


🧪   try this PR locally:

bunx bun-pr 16073

Copy link
Collaborator

@Jarred-Sumner Jarred-Sumner left a comment

Choose a reason for hiding this comment

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

Either one of two things are true:

  • This code causes x to be invalid memory
  • There are two more memory leaks in that code edit: i'm wrong

But I do think it's worth finding out if x is still valid

@DonIsaac
Copy link
Contributor Author

  • PEM_read_bio_X509 is declared by a DECLARE_PEM_rw macro.
  • That function is implemented by IMPLEMENT_PEM_read_bio. It wraps a call to PEM_ASN1_read_bio and casts the returned void pointer.
  • PEM_ASN1_read_bio calls PEM_bytes_read_bio here
  • PEM_bytes_read_bio passes output pointers to PEM_read_bio, and will free those pointers on error
  • PEM_read_bio then allocates memory here

Thus this is a memory leak.

@Jarred-Sumner Jarred-Sumner merged commit ab52058 into main Dec 31, 2024
64 of 67 checks passed
@Jarred-Sumner Jarred-Sumner deleted the don/fix/root-cert-mem-leak branch December 31, 2024 06:20
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.

3 participants