c-hyper: fix two memory leaks in Curl_http.
#11729
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A request created with `hyper_request_new` must be consumed by either `hyper_clientconn_send` or `hyper_request_free`. This is not terrifically clear from the hyper docs -- `hyper_request_free` is documented only with "Free an HTTP request if not going to send it on a client" -- but a perusal of the hyper code confirms it. This commit adds a `hyper_request_free` to the `error:` path in `Curl_http` so that the request is consumed when an error occurs after the request is created but before it is sent. Fixes the first memory leak reported by Valgrind in curl#10803.
There is a `hyper_clientconn_free` call on the happy path, but not one on the error path. This commit adds one. Fixes the second memory leak reported by Valgrind in curl#10803.
Because the leaks have been fixed.
nnethercote
changed the title
Curl_http.Curl_http.
bdaehlie
reviewed
Aug 25, 2023
| @@ -119,12 +119,6 @@ problems may have been fixed or changed somewhat since this was written. | |||
|
|
|||
| 1. HTTP | |||
|
|
|||
| 1.1 hyper memory-leaks | |||
There was a problem hiding this comment.
Probably need to renumber the two entries below and also delete/renumber in the index.
There was a problem hiding this comment.
Nah, the script that converts that to show on the website handles gaps just fine. We prefer gaps than having to reorder every time we add/remove.
bagder
approved these changes
Aug 25, 2023
|
Thanks! |
|
@bagder: In every other GitHub project I've contributed to, accepted PRs end up in the "merged" state, and the "closed" state is used for rejected PRs. But you're doing it in a different way which leaves accepted PRs marked as "Closed with unmerged commits". There is also a message above "bagder closed this in c61dd5f", which refers to a single commit but there were three commits in this PR. Is there a reason for this? As a newbie to the project it surprised me and confused me a bit. Perhaps you don't want merges in the commit history? If that's the case you can allow rebase merging. Or is there another reason? |
|
That's right. Most PRs with pieced changes are condensed into a single commit. The commit message is always amended to meet our guidelines then rebased on upstream/master (upstream: this repo). The author is preserved and the committer name is the person pushing the commit (another guideline). The committer name is not GitHub. |
|
The commit that gets mentioned in the PR is the one that uses the We do this to maintain commit message style, squashed commits and linear history. The commits are done with the command line and GitHub offers no better way to signal this in the PR. I keep pointing this out to them and hope that one day we get a |
nnethercote
added a commit
to nnethercote/hyper
that referenced
this pull request
Aug 28, 2023
Curl had two memory leaks in its usage of the hyper C API. I fixed these in curl/curl#11729. While fixing these, as a hyper newbie I found a lack of clarity in the docs about where the responsibilities lie for allocating and deallocating things. I had to repeatedly look at the `Box::into_raw`/`Box::from_raw` calls in the C API code to properly understand things. This commit extends the docs. - For all functions that allocate things, make it clear which functions can be used to subsequently deallocate. - For all functions that free things, make it clear if there are (or aren't) other functions that may have otherwise consumed the thing, thus removing the need for explicit freeing. - Clarify some functions that consume things. This is the documentation I wished was present when I was fixing the leaks in curl.
nnethercote
added a commit
to nnethercote/hyper
that referenced
this pull request
Aug 28, 2023
Curl had two memory leaks in its usage of the hyper C API. I fixed these in curl/curl#11729. While fixing these, as a hyper newbie I found a lack of clarity in the docs about where the responsibilities lie for allocating and deallocating things. I had to repeatedly look at the `Box::into_raw`/`Box::from_raw` calls in the C API code to properly understand things. This commit extends the docs. - For all functions that allocate things, make it clear which functions can be used to subsequently deallocate. - For all functions that free things, make it clear if there are (or aren't) other functions that may have otherwise consumed the thing, thus removing the need for explicit freeing. - Clarify some functions that consume things. This is the documentation I wished was present when I was fixing the leaks in curl.
nnethercote
added a commit
to nnethercote/hyper
that referenced
this pull request
Aug 30, 2023
Curl had two memory leaks in its usage of the hyper C API. I fixed these in curl/curl#11729. While fixing these, as a hyper newbie I found a lack of clarity in the docs about where the responsibilities lie for allocating and deallocating things. I had to repeatedly look at the `Box::into_raw`/`Box::from_raw` calls in the C API code to properly understand things. This commit extends the docs. - For all functions that allocate things, make it clear which functions can be used to subsequently deallocate. - For all functions that free things, make it clear if there are (or aren't) other functions that may have otherwise consumed the thing, thus removing the need for explicit freeing. - Clarify some functions that consume things. This is the documentation I wished was present when I was fixing the leaks in curl.
nnethercote
added a commit
to nnethercote/hyper
that referenced
this pull request
Aug 30, 2023
Curl had two memory leaks in its usage of the hyper C API. I fixed these in curl/curl#11729. While fixing these, as a hyper newbie I found a lack of clarity in the docs about where the responsibilities lie for allocating and deallocating things. I had to repeatedly look at the `Box::into_raw`/`Box::from_raw` calls in the C API code to properly understand things. This commit extends the docs. - For all functions that allocate things, make it clear which functions can be used to subsequently deallocate. - For all functions that free things, make it clear if there are (or aren't) other functions that may have otherwise consumed the thing, thus removing the need for explicit freeing. - Clarify some functions that consume things. This is the documentation I wished was present when I was fixing the leaks in curl.
seanmonstar
pushed a commit
to hyperium/hyper
that referenced
this pull request
Aug 31, 2023
Curl had two memory leaks in its usage of the hyper C API. I fixed these in curl/curl#11729. While fixing these, as a hyper newbie I found a lack of clarity in the docs about where the responsibilities lie for allocating and deallocating things. I had to repeatedly look at the `Box::into_raw`/`Box::from_raw` calls in the C API code to properly understand things. This commit extends the docs. - For all functions that allocate things, make it clear which functions can be used to subsequently deallocate. - For all functions that free things, make it clear if there are (or aren't) other functions that may have otherwise consumed the thing, thus removing the need for explicit freeing. - Clarify some functions that consume things. This is the documentation I wished was present when I was fixing the leaks in curl.
ptitSeb
pushed a commit
to wasix-org/curl
that referenced
this pull request
Sep 25, 2023
A request created with `hyper_request_new` must be consumed by either `hyper_clientconn_send` or `hyper_request_free`. This is not terrifically clear from the hyper docs -- `hyper_request_free` is documented only with "Free an HTTP request if not going to send it on a client" -- but a perusal of the hyper code confirms it. This commit adds a `hyper_request_free` to the `error:` path in `Curl_http` so that the request is consumed when an error occurs after the request is created but before it is sent. Fixes the first memory leak reported by Valgrind in curl#10803. Closes curl#11729
ptitSeb
pushed a commit
to wasix-org/curl
that referenced
this pull request
Sep 25, 2023
There is a `hyper_clientconn_free` call on the happy path, but not one on the error path. This commit adds one. Fixes the second memory leak reported by Valgrind in curl#10803. Fixes curl#10803 Closes curl#11729
0xE282B0
pushed a commit
to 0xE282B0/hyper
that referenced
this pull request
Jan 12, 2024
Curl had two memory leaks in its usage of the hyper C API. I fixed these in curl/curl#11729. While fixing these, as a hyper newbie I found a lack of clarity in the docs about where the responsibilities lie for allocating and deallocating things. I had to repeatedly look at the `Box::into_raw`/`Box::from_raw` calls in the C API code to properly understand things. This commit extends the docs. - For all functions that allocate things, make it clear which functions can be used to subsequently deallocate. - For all functions that free things, make it clear if there are (or aren't) other functions that may have otherwise consumed the thing, thus removing the need for explicit freeing. - Clarify some functions that consume things. This is the documentation I wished was present when I was fixing the leaks in curl.
0xE282B0
pushed a commit
to 0xE282B0/hyper
that referenced
this pull request
Jan 16, 2024
Curl had two memory leaks in its usage of the hyper C API. I fixed these in curl/curl#11729. While fixing these, as a hyper newbie I found a lack of clarity in the docs about where the responsibilities lie for allocating and deallocating things. I had to repeatedly look at the `Box::into_raw`/`Box::from_raw` calls in the C API code to properly understand things. This commit extends the docs. - For all functions that allocate things, make it clear which functions can be used to subsequently deallocate. - For all functions that free things, make it clear if there are (or aren't) other functions that may have otherwise consumed the thing, thus removing the need for explicit freeing. - Clarify some functions that consume things. This is the documentation I wished was present when I was fixing the leaks in curl. Signed-off-by: Sven Pfennig <s.pfennig@reply.de>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These commits fix the memory leaks in #10803.