Close response body in error case and close first one

[mholt]

I noticed that the response body is not closed in the error case of waiting for a certificate to be issued: https://sourcegraph.com/github.com/xenolf/lego@ce8fb060cb8361a9ff8b5fb7c2347fa907b6fcac/-/blob/acme/client.go#L673-674

AFAIK the response body needs to be closed even if there isn't one or it is not read. We close it above if the response code is 201 or 202, but not 200 or anything else. I think this will fix that problem.

[mholt]

Incidentally, I think there may be one more subtle leak, but I want someone else to verify.

On this line we initially get an *http.Response, which has a body that must be closed. I think the body for that very first response gets closed if and only if it comes back with a 201 or 202 status code. If it doesn't, we replace resp with a new one here but we never closed the body of the first one.

I actually caught this thanks to Sourcegraph's code browser; when I clicked the resp variable, it took me up to that first line where I saw that we were re-using an earlier variable, not creating a new one.

Am I missing something or is that also a bug?

[mholt]

Actually, I'm not sure if my initial PR was entirely correct. There are some places where handleHTTPError is called after a defer resp.Body.Close(), meaning in those cases, the body will be closed twice. I just pushed an amendment that should defer the Close() in the right place without duplicating/overlapping Close() calls.

[miekg]

Why not put this defer directly after the post, i.e. line 612? Then the Close in the other switch can be removed as well.

But yeah, def. a leak here.

[mholt]

@miekg Because it might be read in line 621. If that's never the case, then sure, but I don't want to break things. I need feedback from @xenolf.

[mholt]

@xenolf @miekg I've pushed a commit that ensures the body for both responses is closed. I had to pull the select out into its own function in order for it to be reasonably readable.

I also took 1024*1024 into a constant, and set a cap for the number of iterations (polling) we're allowed to do in checking for a certificate. There's no reason it should take 1000 polls to get a single cert, and it will be one extra layer of protection against some buggy protocol implementation, perhaps.

Tested and it works :) Please review ASAP. (Will go forward including this in new Caddy releases either way, though.)

[xenolf]

Hey @mholt thanks for catching that. The latest revision of the changes is looking good.

[xenolf]

Could we move that up to the top? :)

[mholt]

Sure! Will do in a little bit. (Edit: Done!)