Re: [PATCH v2 5/5] http: reset CURLOPT_POSTFIELDSIZE_LARGE between requests

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2714 bytes --]

On Tue, Nov 14, 2023 at 07:34:55PM -0800, Jiří Hruška wrote:
> `get_active_slot()` makes sure that the reused cURL handles it gives
> out are as good as fresh ones, by resetting all options that other code
> might have set on them back to defaults.
> 
> But this does not apply to `CURLOPT_POSTFIELDSIZE_LARGE` yet, which can
> stay set from a previous request. For example, an earlier probe request
> with just a flush packet "0000" leaves it set to 4.
> 
> The problem seems harmless in practice, but it can be confusing to see
> a negative amount of remaining bytes to upload when inspecting libcurl
> internals while debugging networking-related issues, for example.
> 
> So reset also this option to its default value (which is -1, not 0).
> 
> Signed-off-by: Jiri Hruska <jirka@fud.cz>
> ---
>  http.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/http.c b/http.c
> index 8f71bf00d8..14f2fbb82e 100644
> --- a/http.c
> +++ b/http.c
> @@ -1454,6 +1454,7 @@ struct active_request_slot *get_active_slot(void)
>  	curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, NULL);
>  	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, NULL);
>  	curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL);
> +	curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE_LARGE, (curl_off_t)-1);
>  	curl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);
>  	curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
>  	curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);

It feels quite easy for this list to grow stale whenever we start to set
a new option somewhere else. Is there a specific reason why we can't
instead use `curl_easy_reset()` here? Quoting its description:

> Re-initializes all options previously set on a specified CURL handle
> to the default values. This puts back the handle to the same state as
> it was in when it was just created with curl_easy_init.
> 
> It does not change the following information kept in the handle: live
> connections, the Session ID cache, the DNS cache, the cookies, the
> shares or the alt-svc cache.

From my naive point of view it sounds like exactly what we're after.
Most of the code in question was introduced in 9094950d73 (http: prevent
segfault during curl handle reuse, 2006-05-31), where we used to support
libcurl at least back to v7.7. `curl_easy_reset()` on the other hand had
only been introduced with v7.12.1 of libcurl, so maybe that's the reason
why it's not used here?

I dunno, might as well be that there is a good reason why we don't use
it here. But if we can, then I'd argue it would be a great cleanup to
convert to `curl_easy_reset()` here instead of piling onto the list of
options.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]