Re: [PATCH v5] http.postbuffer: allow full range of ssize_t values

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 4, 2017, 01:32 UTC
Message-ID: <20170404013210.GD8741@aiede.mtv.corp.google.com>
In-Reply-To: <20170403235308.30102-1-dturner@twosigma.com>

David Turner wrote:

> This means that we need to use CURLOPT_POSTFIELDSIZE_LARGE to set the
> buffer size.

Neat.

For completeness, it's useful to know this was added in curl 7.11.1, which is old enough for us to be able to count on users having it (in fact it was released >10 years ago).

[...]

Show 8 quoted lines

> +++ b/remote-curl.c
> @@ -531,6 +531,12 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
>  	return err;
>  }
>  
> +static curl_off_t xcurl_off_t(ssize_t len) {
> +	if (len > (curl_off_t) len)
> +		die("Cannot handle pushes this big");

nit: other calls to die() here and elsewhere tend to use a lowercase error message.

More importantly, converting a value to a signed type when the value cannot be represented in it yields implementation-defined behavior (C99 section 6.3.1.3 "signed and unsigned integers"). That makes it fodder for over-eager optimizers.

Would something like the following work?

With that change,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

diff --git i/remote-curl.c w/remote-curl.c
index b7b69e096a..cf171b1bc9 100644
--- i/remote-curl.c
+++ w/remote-curl.c
@@ -532,8 +532,8 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 }
 
 static curl_off_t xcurl_off_t(ssize_t len) {
-	if (len > (curl_off_t) len)
-		die("Cannot handle pushes this big");
+	if (len > maximum_signed_value_of_type(curl_off_t))
+		die("cannot handle pushes this big");
 	return (curl_off_t) len;
 }