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

Jump to bottom

http2: fix handle leak in error path #1416

Closed
wants to merge 1 commit into from
Closed

http2: fix handle leak in error path #1416

wants to merge 1 commit into from

Conversation

larrystefanisonos
Copy link
Contributor

Add missing newhandle free call in push_promise().

@larrystefanisonos
Update http2.c
c675619 
Add missing newhandle free call in push_promise().
@mention-bot
Copy link

@lstefani, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @tatsuhiro-t and @jay to be potential reviewers.

@bagder bagder changed the title Update http2.c http2: fix handle leak in error path Apr 13, 2017
bagder
bagder reviewed Apr 13, 2017
View reviewed changes
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

Good catch, but it seems to me that moving the Curl_close(newhandle); call to the fail: label is a better fix, and then we can remove it from the other error checks that goto fail. What do you think?

@larrystefanisonos
Copy link
Contributor Author

I wouldn't recommend that, based on the current placement of the fail: label (it's a drop-through for good cases). However, you could add a return rv; right before the fail: label to take care of the drop-through (non-goto cases). Also, notice that we goto on alloc fail, so you'd need to confirm that it's safe to call Curl_close() with a NULL pointer.

@bagder bagder closed this in 1451271 Apr 15, 2017
@bagder
Copy link
Member

bagder commented Apr 15, 2017

Right, and newhandle had a different scope too. I decided to let your change be and merge that. Thanks!

@lock lock bot locked as resolved and limited conversation to collaborators May 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bagder bagder bagder left review comments

Assignees
No one assigned
Labels
HTTP/2 memory-leak
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@larrystefanisonos @mention-bot @bagder