Fix curl_easy_setopt() parameter type problem, again

[dscho]

As of last week, every CI build of Git for Windows' ARM64 flavor of its SDK started failing (the first failed build is this here: https://github.com/git-for-windows/git-sdk-arm64/actions/runs/17633130672/job/50104373185).

The reason is that we still have one curl_easy_setopt() call that is supposed to pass a long parameter but doesn't.

The likely explanation is an update to libcurl in the Git for Windows/ARM64 SDK. A strange thing about this: GCC does not catch the problem, but clang does. And even more strangely, the relevant parts of cURL's typecheck-gcc.h haven't changed in a way that explains to me why this starts to trigger now.

To prevent more piecemeal solutions, I vetted all calls of curl_easy_setopt() and satisified myself that all remaining calls that want long parameters do indeed have arguments matching that type.

As is the custom in the Git project, I use this opportunity to do a bit more than is strictly necessary to address this problem. I replace the affected call with one that expects a wider data type, refactor an already-existing function to cast to said data type, and use it in the modified call as well as in another call which wants to perform the same cast but did so unsafely (not that it mattered in practice, really, yet it is in line with other refactors the Git project has seen).

Note for the curious: The affected call used CURLOPT_INFILESIZE and now uses CURLOPT_INFILESIZE_LARGE, and it is a bit strange that the latter wasn't used in the first place, given that it was introduced into cURL over a year before Git itself was born, and almost two years before the call was introduced in 58e60dd (Add support for pushing to a remote repository using HTTP/DAV, 2005-11-02).

For the record, these patches apply cleanly all the way back to v2.22, according to git replay. I did not try to test whether it builds, though, because all kinds of stunts are required nowadays to build this old versions even without any patches on top.

[dscho]

/submit

[gitgitgadget]

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1974/dscho/curl-easy-setopt-typefix-again-v1

To fetch this version to local tag pr-1974/dscho/curl-easy-setopt-typefix-again-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1974/dscho/curl-easy-setopt-typefix-again-v1

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <[email protected]>
writes:

> From: Johannes Schindelin <[email protected]>
>
> This commit moves the `xcurl_off_t()` function, which validates that a
> given value fits within the `curl_off_t` data type and then casts it, to
> a more central place so that it can be used outside of `remote-curl.c`,
> too.
>
> At the same time, this function is renamed to conform better with the
> naming convention of the helper functions that safely cast from one data
> type to another which has been well established in `git-compat-util.h`.

OK.  The code inside the renamed function is the same with an
updated message to show the value.

> With this move, the error message can unfortunately no longer be renamed
> because the `_(...)` function is not available at the time of
> definition.

It is not clear to me what change (or lack thereof?) in this patch
this paragraph refers to.  Who wants to rename what error message
and why?

> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
>  http.h        | 10 ++++++++++
>  remote-curl.c | 14 +++-----------
>  2 files changed, 13 insertions(+), 11 deletions(-)

Thanks; will queue.

> +static inline curl_off_t cast_size_t_to_curl_off_t(size_t a)
> +{
> +	uintmax_t size = a;
> +	if (size > maximum_signed_value_of_type(curl_off_t))
> +		die(_("number too large to represent as curl_off_t "
> +		      "on this platform: %"PRIuMAX), (uintmax_t)a);
> +	return (curl_off_t)a;
> +}
> +
> -static curl_off_t xcurl_off_t(size_t len)
> -{
> -	uintmax_t size = len;
> -	if (size > maximum_signed_value_of_type(curl_off_t))
> -		die(_("cannot handle pushes this big"));
> -	return (curl_off_t)size;
> -}

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <[email protected]>
writes:

> From: Johannes Schindelin <[email protected]>
>
> When casting a `size_t` to `curl_off_t`, there is a currently uncommon
> chance that the value can be cut off (`curl_off_t` is supposed to be
> guaranteed to be 64-bit).

"64-bit" -> "signed 64-bit", per what <curl/system.h> says,
i.e. "curl_off_t MUST be typedef'ed to a 64-bit * wide signed
integral data type.", perhaps?

msgbuf.buf is a strbuf, so its .len member is of size_t (so is
prev_len); prev_len is how big the msgbuf.buf was before the code
added some stuff taken from all_msgs, possibly wrapping the result
in html, and converting lf to crlf, all only enlarging the buffer.
We are computing how much we bloated the msgbuf.buf relative to the
original here, so this subtraction should not suffer unsigned
wraparound.  Now thanks to the previous step, we can consistently
and safely cast size_t values to curl_off_t easily, which is nice.

Looking good.  Will queue.  Thanks.

> diff --git a/imap-send.c b/imap-send.c
> index 4bd5b8aa0d..26dda7f328 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -1721,7 +1721,7 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,
>  		lf_to_crlf(&msgbuf.buf);
>  
>  		curl_easy_setopt(curl, CURLOPT_INFILESIZE_LARGE,
> -				 (curl_off_t)(msgbuf.buf.len-prev_len));
> +				 cast_size_t_to_curl_off_t(msgbuf.buf.len-prev_len));
>  
>  		res = curl_easy_perform(curl);

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <[email protected]>
writes:

> However, there is a much better solution: To use the
> `CURLOPT_INFILESIZE_LARGE` option instead, which was added in cURL
> v7.11.0 (see https://curl.se/ch/7.11.0.html) and which Git _already_
> uses in `curl_append_msgs_to_imap()`.
>
> This fix was the motivation for renaming `xcurl_off_t()` to
> `cast_size_t_to_curl_off_t()` and making it available more broadly,
> which is the reason why it is used here, too.
>
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
>  http-push.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

As we saw in [1/3], we have been using "size large" for posting in
remote-curl.c and this brings how the size for reading the data is
handled to match that.

Looking good.  

Will queue.  Thanks.



> diff --git a/http-push.c b/http-push.c
> index 91a5465afb..7a9b96a6d0 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -208,7 +208,8 @@ static void curl_setup_http(CURL *curl, const char *url,
>  	curl_easy_setopt(curl, CURLOPT_UPLOAD, 1L);
>  	curl_easy_setopt(curl, CURLOPT_URL, url);
>  	curl_easy_setopt(curl, CURLOPT_INFILE, buffer);
> -	curl_easy_setopt(curl, CURLOPT_INFILESIZE, buffer->buf.len);
> +	curl_easy_setopt(curl, CURLOPT_INFILESIZE_LARGE,
> +			 cast_size_t_to_curl_off_t(buffer->buf.len));
>  	curl_easy_setopt(curl, CURLOPT_READFUNCTION, fread_buffer);
>  	curl_easy_setopt(curl, CURLOPT_SEEKFUNCTION, seek_buffer);
>  	curl_easy_setopt(curl, CURLOPT_SEEKDATA, buffer);

[gitgitgadget]

This patch series was integrated into seen via git@44f0514.