[PATCH v1 2/2] curl_off_t xcurl_off_t is not portable

[PATCH v1 2/2] curl_off_t xcurl_off_t is not portable

[tboegi]

From: Torsten Bögershausen <tboegi@web.de>

Comparing signed and unsigned values is not always portable.
When  setting
DEVELOPER = 1
DEVOPTS = extra-all

"gcc (Raspbian 6.3.0-18+rpi1+deb9u1) 6.3.0 20170516" errors out with
"comparison is always false due to limited range of data type"
"[-Werror=type-limits]"

Solution:
Use a valid cast & compare, similar to xsize_t()

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 remote-curl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 762a55a75f..c89fd6d1c3 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -618,9 +618,10 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 }
 
 static curl_off_t xcurl_off_t(ssize_t len) {
-	if (len > maximum_signed_value_of_type(curl_off_t))
+	curl_off_t size = (curl_off_t) len;
+	if (len != (ssize_t) size)
 		die("cannot handle pushes this big");
-	return (curl_off_t) len;
+	return size;
 }
 
 static int post_rpc(struct rpc_state *rpc)
-- 
2.11.0

Re: [PATCH v1 2/2] curl_off_t xcurl_off_t is not portable

[Junio C Hamano]

tboegi@web.de writes:

> From: Torsten Bögershausen <tboegi@web.de>

> Subject: Re: [PATCH v1 2/2] curl_off_t xcurl_off_t is not portable

That title is misleading; it sounded as if the are these two
typedefs and they do not work correctly on some platforms, but that
is not what you are doing with the patch.

> Comparing signed and unsigned values is not always portable.

Is that what the compiler is complaining about?  There is this bit
in git-compat-util.h:

/*
 * Signed integer overflow is undefined in C, so here's a helper macro
 * to detect if the sum of two integers will overflow.
 *
 * Requires: a >= 0, typeof(a) equals typeof(b)
 */
#define signed_add_overflows(a, b) \
    ((b) > maximum_signed_value_of_type(a) - (a))

which is designed to be fed signed a and signed b.  The macro is
used in packfile codepaths to compare int, off_t, etc..

So the statement may be true but it does not seem to have much to do
with the problem you are seeing with maximum_signed_value_of_type().

> When  setting
> DEVELOPER = 1
> DEVOPTS = extra-all
>
> "gcc (Raspbian 6.3.0-18+rpi1+deb9u1) 6.3.0 20170516" errors out with
> "comparison is always false due to limited range of data type"
> "[-Werror=type-limits]"

Then this sounds a bit different from "comparison between signed
ssize_t len and unsigned maximum_signed_value_of_type() is bad".
Isn't it saying that "No matter how big you make len, you can never
go beyond maximum_signed_value_of_type(curl_off_t)"?

> diff --git a/remote-curl.c b/remote-curl.c
> index 762a55a75f..c89fd6d1c3 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -618,9 +618,10 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
>  }
>  
>  static curl_off_t xcurl_off_t(ssize_t len) {
> -	if (len > maximum_signed_value_of_type(curl_off_t))

Is the issue that len is signed and maximum_signed_value_of_type()
gives an unsigned value, and these two are compared?  As we saw
earlier, signed_add_overflows() is another example that wants a
mixed comparison.

I am just wondering if casting len to uintmax_t before comparing
with maximum_signed_value_of_type() is a simpler solution that can
safely be cargo-culted to other places without much thinking.

"git grep maximum_signed_value_of_type" reports a handful
comparisons in vcs-svn/, all of which does

	if (var > maximum_signed_value_of_type(off_t))

with var of type uintmax_t, which sounds like a sane thing to do.

Thanks.

> +	curl_off_t size = (curl_off_t) len;
> +	if (len != (ssize_t) size)
>  		die("cannot handle pushes this big");
> -	return (curl_off_t) len;
> +	return size;
>  }

Re: [PATCH v1 2/2] curl_off_t xcurl_off_t is not portable

[Torsten Bögershausen]

On Fri, Oct 26, 2018 at 11:48:38AM +0900, Junio C Hamano wrote:
> tboegi@web.de writes:
> 
> > From: Torsten Bögershausen <tboegi@web.de>
> 
> > Subject: Re: [PATCH v1 2/2] curl_off_t xcurl_off_t is not portable
> 
> That title is misleading; it sounded as if the are these two
> typedefs and they do not work correctly on some platforms, but that
> is not what you are doing with the patch.

OK.

> 
> > Comparing signed and unsigned values is not always portable.
> 
> Is that what the compiler is complaining about?  There is this bit
> in git-compat-util.h:

No, not that either, see below.

> 
> /*
>  * Signed integer overflow is undefined in C, so here's a helper macro
>  * to detect if the sum of two integers will overflow.
>  *
>  * Requires: a >= 0, typeof(a) equals typeof(b)
>  */
> #define signed_add_overflows(a, b) \
>     ((b) > maximum_signed_value_of_type(a) - (a))
> 
> which is designed to be fed signed a and signed b.  The macro is
> used in packfile codepaths to compare int, off_t, etc..
> 
> So the statement may be true but it does not seem to have much to do
> with the problem you are seeing with maximum_signed_value_of_type().
> 
> > When  setting
> > DEVELOPER = 1
> > DEVOPTS = extra-all
> >
> > "gcc (Raspbian 6.3.0-18+rpi1+deb9u1) 6.3.0 20170516" errors out with
> > "comparison is always false due to limited range of data type"
> > "[-Werror=type-limits]"
> 
> Then this sounds a bit different from "comparison between signed
> ssize_t len and unsigned maximum_signed_value_of_type() is bad".
> Isn't it saying that "No matter how big you make len, you can never
> go beyond maximum_signed_value_of_type(curl_off_t)"?

I digged a little bit deeper into the raspi, and this is what I find
under
/usr/include/arm-linux-gnueabihf/curl

curlbuild.h:#define CURL_TYPEOF_CURL_OFF_T int64_t
curlbuild.h:typedef CURL_TYPEOF_CURL_OFF_T curl_off_t;

> 
> > diff --git a/remote-curl.c b/remote-curl.c
> > index 762a55a75f..c89fd6d1c3 100644
> > --- a/remote-curl.c
> > +++ b/remote-curl.c
> > @@ -618,9 +618,10 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
> >  }
> >  
> >  static curl_off_t xcurl_off_t(ssize_t len) {
> > -	if (len > maximum_signed_value_of_type(curl_off_t))
> 
> Is the issue that len is signed and maximum_signed_value_of_type()
> gives an unsigned value, and these two are compared?  As we saw
> earlier, signed_add_overflows() is another example that wants a
> mixed comparison.
> 
> I am just wondering if casting len to uintmax_t before comparing
> with maximum_signed_value_of_type() is a simpler solution that can
> safely be cargo-culted to other places without much thinking.

I don't know.
Since ssize_t is 32 bit on the raspi, and curl_off_t is 64 bit,
the test seems not to be needed at all ;-)
I don't know if it makes sense to stop thinking here and if
casting to uintmax_t is the right solution here.

And, I like the easy-to-read xsize_t, which is safe and warm.
Agreed that the commit message is wrong.
I would like to keep the xsize_t aproach, are there more thoughts ?

> 
> "git grep maximum_signed_value_of_type" reports a handful
> comparisons in vcs-svn/, all of which does
> 
> 	if (var > maximum_signed_value_of_type(off_t))
> 
> with var of type uintmax_t, which sounds like a sane thing to do.
> 
> Thanks.
> 
> > +	curl_off_t size = (curl_off_t) len;
> > +	if (len != (ssize_t) size)
> >  		die("cannot handle pushes this big");
> > -	return (curl_off_t) len;
> > +	return size;
> >  }
> 

[PATCH v2 1/1] remote-curl.c: xcurl_off_t is not portable (on 32 bit platfoms)

[tboegi]

From: Torsten Bögershausen <tboegi@web.de>

When  setting
DEVELOPER = 1
DEVOPTS = extra-all

"gcc (Raspbian 6.3.0-18+rpi1+deb9u1) 6.3.0 20170516" errors out with
"comparison is always false due to limited range of data type"
"[-Werror=type-limits]"

It turns out that the function xcurl_off_t() has 2 flavours:
- It gives a warning 32 bit systems, like Linux
- It takes the signed ssize_t as a paramter, but the only caller is using
  a size_t (which is typically unsigned these days)

The original motivation of this function is to make sure that sizes > 2GiB
are handled correctly. The curl documentation says:
"For any given platform/compiler curl_off_t must be typedef'ed to a 64-bit
 wide signed integral data type"
On a 32 bit system "size_t" can be promoted into a 64 bit signed value
without loss of data, and therefore we may see the
"comparison is always false" warning.
On a 64 bit system it may happen, at least in theory, that size_t is > 2^63,
and then the promotion from an unsigned "size_t" into a signed "curl_off_t"
may be a problem.

One solution to suppress a possible compiler warning could be to remove
the function xcurl_off_t().

However, to be on the very safe side, we keep it and improve it:
- The len parameter is changed from ssize_t to size_t
- A temporally variable "size" is used, promoted int uintmax_t and the comopared
  with "maximum_signed_value_of_type(curl_off_t)".
  Thanks to Junio C Hamano for this hint.

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 remote-curl.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 762a55a75f..1220dffcdc 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -617,10 +617,11 @@ 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 > maximum_signed_value_of_type(curl_off_t))
+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) len;
+	return (curl_off_t)size;
 }
 
 static int post_rpc(struct rpc_state *rpc)
-- 
2.19.0.271.gfe8321ec05

[PATCH v2 1/1] remote-curl.c: xcurl_off_t is not portable (on 32 bit platfoms)

[tboegi]

From: Torsten Bögershausen <tboegi@web.de>

When  setting
DEVELOPER = 1
DEVOPTS = extra-all

"gcc (Raspbian 6.3.0-18+rpi1+deb9u1) 6.3.0 20170516" errors out with
"comparison is always false due to limited range of data type"
"[-Werror=type-limits]"

It turns out that the function xcurl_off_t() has 2 flavours:
- It gives a warning 32 bit systems, like Linux
- It takes the signed ssize_t as a paramter, but the only caller is using
  a size_t (which is typically unsigned these days)

The original motivation of this function is to make sure that sizes > 2GiB
are handled correctly. The curl documentation says:
"For any given platform/compiler curl_off_t must be typedef'ed to a 64-bit
 wide signed integral data type"
On a 32 bit system "size_t" can be promoted into a 64 bit signed value
without loss of data, and therefore we may see the
"comparison is always false" warning.
On a 64 bit system it may happen, at least in theory, that size_t is > 2^63,
and then the promotion from an unsigned "size_t" into a signed "curl_off_t"
may be a problem.

One solution to suppress a possible compiler warning could be to remove
the function xcurl_off_t().

However, to be on the very safe side, we keep it and improve it:
- The len parameter is changed from ssize_t to size_t
- A temporally variable "size" is used, promoted int uintmax_t and the comopared
  with "maximum_signed_value_of_type(curl_off_t)".
  Thanks to Junio C Hamano for this hint.

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---

This is a re-semd, the orignal patch was part of a 2
patch-series.
This patch needed some rework, and here should be
the polished version.

remote-curl.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 762a55a75f..1220dffcdc 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -617,10 +617,11 @@ 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 > maximum_signed_value_of_type(curl_off_t))
+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) len;
+	return (curl_off_t)size;
 }
 
 static int post_rpc(struct rpc_state *rpc)
-- 
2.19.0.271.gfe8321ec05

Re: [PATCH v2 1/1] remote-curl.c: xcurl_off_t is not portable (on 32 bit platfoms)

[Junio C Hamano]

tboegi@web.de writes:

>
> This is a re-semd, the orignal patch was part of a 2
> patch-series.
> This patch needed some rework, and here should be
> the polished version.

Will queue.  Next time, please refrain from saying "re-send", if you
changed anything in the patch (or the log message), as the phrase
implies you are sending the same thing as before (just in case the
earlier one was not seen, for example).  Marking it as vN+1 like you
did for this patch and having a reference to the original would make
it clear, though ;-)

Thanks.

> remote-curl.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/remote-curl.c b/remote-curl.c
> index 762a55a75f..1220dffcdc 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -617,10 +617,11 @@ 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 > maximum_signed_value_of_type(curl_off_t))
> +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) len;
> +	return (curl_off_t)size;
>  }
>  
>  static int post_rpc(struct rpc_state *rpc)

Re: [PATCH v2 1/1] remote-curl.c: xcurl_off_t is not portable (on 32 bit platfoms)

[Torsten Bögershausen]

On Mon, Nov 12, 2018 at 12:50:30PM +0900, Junio C Hamano wrote:
> tboegi@web.de writes:
> 
> >
> > This is a re-semd, the orignal patch was part of a 2
> > patch-series.
> > This patch needed some rework, and here should be
> > the polished version.
> 
> Will queue.

Thanks, is there a chance to kill a typo here ?
s/comopared/compared/
- A temporally variable "size" is used, promoted int uintmax_t and the comopared


> Next time, please refrain from saying "re-send", if you
> changed anything in the patch (or the log message), as the phrase
> implies you are sending the same thing as before (just in case the
> earlier one was not seen, for example).  Marking it as vN+1 like you
> did for this patch and having a reference to the original would make
> it clear, though ;-)

Sorry for the confusion.
The next time I will not send unrelated patches as a series,
so that we have a better "Message-ID:" and "In-Reply-To:"
flow (which should make live 3% easier).

https://public-inbox.org/git/20181029165914.2677-1-tboegi@web.de/
https://public-inbox.org/git/20181109174110.27630-1-tboegi@web.de/

Re: [PATCH v2 1/1] remote-curl.c: xcurl_off_t is not portable (on 32 bit platfoms)

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> Thanks, is there a chance to kill a typo here ?
> s/comopared/compared/
> - A temporally variable "size" is used, promoted int uintmax_t and the comopared

Done.  Thanks.