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

net/http/httputil: DumpResponse could corrupt Response object #14036

Closed
Loofort opened this issue Jan 20, 2016 · 4 comments
Closed

net/http/httputil: DumpResponse could corrupt Response object #14036

Loofort opened this issue Jan 20, 2016 · 4 comments

Comments

@Loofort
Copy link

Loofort commented Jan 20, 2016

Here is a code snippet used inside DumpResponse and DumpResponseOut functions:

    save, resp.Body, err = drainBody(resp.Body)
    if err != nil {
        return
    }

In case when resp.Body is corrupted, drainBody returns error and resp.Body is assigned to nil. Then when caller try to execute resp.Body.Close() it will panic.
That is why e.g. this legal code might panic:

    response, err := httpClient.Do(request)
    if err != nil {
        return err
    }
    defer response.Body.Close()

    dump, err := httputil.DumpResponse(response, true)
    if err == nil {
        return err
    }
@bradfitz
Copy link
Contributor

That's not how defer works. Defer evaluates everything needed for the call but doesn't make the call until later. But "response.Body" is fully evaluated before DumpResponse:

Example for you to play with: http://play.golang.org/p/AE6mwTOV6j

Maybe you meant http://play.golang.org/p/YIOKnNMT-N ?

In any case, this seems like fine behavior. If httputil.DumpResponse returns an error after consuming the body, it seems valid to assume that res.Body is messed up. And code like my first link is much more common than code like my second link.

@Loofort
Copy link
Author

Loofort commented Jan 20, 2016

You right about defer,
I was trying to simplify the code and made mistake.

our real ussage is that we use DumpResponse (for logging purpose) before response.Body.Close(), and don't check dump err.

I was surprized that DumpResponse might assign nil to resp.Body
If it is not bug but feature, it's better to put the notice to documentation.

Thanks.

@bradfitz
Copy link
Contributor

Sure, I'll document.

@bradfitz bradfitz reopened this Jan 20, 2016
@gopherbot
Copy link

CL https://golang.org/cl/18726 mentions this issue.

@bradfitz bradfitz self-assigned this Jan 21, 2016
@bradfitz bradfitz added this to the Go1.6Maybe milestone Jan 21, 2016
@golang golang locked and limited conversation to collaborators Jan 24, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants