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

http2: fix handle leak in error path #1416

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@lstefani
Contributor

lstefani commented Apr 13, 2017

Add missing newhandle free call in push_promise().

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

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Apr 13, 2017

@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.

mention-bot commented Apr 13, 2017

@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 from Update http2.c to http2: fix handle leak in error path Apr 13, 2017

@bagder

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?

@lstefani

This comment has been minimized.

Show comment
Hide comment
@lstefani

lstefani Apr 13, 2017

Contributor

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.

Contributor

lstefani commented Apr 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Apr 15, 2017

Member

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

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.