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

Prevent leak on error responses #454

Merged

Conversation

ChuckCrawford
Copy link
Collaborator

@ChuckCrawford ChuckCrawford commented Aug 4, 2022

Reverts #453 and instead applies the fix in a common location.

This reverts commit 0148d77.
@ChuckCrawford ChuckCrawford marked this pull request as ready for review August 4, 2022 12:35
Copy link
Contributor

@acloudgopher-pd acloudgopher-pd left a comment

Choose a reason for hiding this comment

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

Looks ok to me all tests pass, but I'm a bit curious why only check for non json content

client.go Outdated Show resolved Hide resolved
@ChuckCrawford
Copy link
Collaborator Author

Looks ok to me all tests pass, but I'm a bit curious why only check for non json content

Ah good question @acloudgopher-pd. I should have left some comments describing my thought process. So all of the requests eventually call c.checkResponse(resp, err) which has some common handling of non-2xx status codes:

go-pagerduty/client.go

Lines 583 to 585 in 8895770

if resp.StatusCode < 200 || resp.StatusCode > 299 {
return resp, c.getErrorFromResponse(resp)
}

inside of getErrorFromResponse, the call to decodeJSON has handling to close the response body:

go-pagerduty/client.go

Lines 560 to 566 in 8895770

func (c *Client) decodeJSON(resp *http.Response, payload interface{}) error {
// close the original response body, and not the copy we may make if
// debugCaptureResponse is true
orb := resp.Body
defer func() { _ = orb.Close() }() // explicitly discard error
body, err := ioutil.ReadAll(resp.Body)

so it looked like the only place this was missing was for the scenario where a non-JSON response was returned...

Copy link
Contributor

@rafusel rafusel left a comment

Choose a reason for hiding this comment

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

This looks like a better approach to me 👍

@ChuckCrawford ChuckCrawford merged commit e23c1d0 into PagerDuty:master Aug 5, 2022
@ChuckCrawford ChuckCrawford deleted the bg/leak-on-error-response branch August 5, 2022 12:20
@ChuckCrawford ChuckCrawford added this to the v1.6.0 milestone Sep 21, 2022
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.

None yet

3 participants