[PATCH 1/2] Add strtoimax() compatibility function.

[PATCH 1/2] Add strtoimax() compatibility function.

[Nix]

Since systems that omit strtoumax() will likely omit strtomax() too,
and likewise for strtoull() and strtoll(), we also adjust the
compatibility #defines from NO_STRTOUMAX to NO_STRTOMAX and from
NO_STRTOULL to NO_STRTOLL, and have them cover both the signed and
unsigned functions.

Signed-off-by: Nick Alcock <nix@esperi.org.uk>
---
 Makefile           |   36 ++++++++++++++++++------------------
 compat/strtoimax.c |   10 ++++++++++
 compat/strtoumax.c |    2 +-
 3 files changed, 29 insertions(+), 19 deletions(-)
 create mode 100644 compat/strtoimax.c

diff --git a/Makefile b/Makefile
index 6bf7d6c..0959e07 100644
--- a/Makefile
+++ b/Makefile
@@ -57,9 +57,9 @@ all::
 #
 # Define NO_STRLCPY if you don't have strlcpy.
 #
-# Define NO_STRTOUMAX if you don't have strtoumax in the C library.
-# If your compiler also does not support long long or does not have
-# strtoull, define NO_STRTOULL.
+# Define NO_STRTOMAX if you don't have both strtoimax and strtoumax in the
+# C library.  If your compiler also does not support long long or does
+# not have strtoll or strtoull, define NO_STRTOLL.
 #
 # Define NO_SETENV if you don't have setenv in the C library.
 #
@@ -804,7 +804,7 @@ ifeq ($(uname_S),OSF1)
 	# Need this for u_short definitions et al
 	BASIC_CFLAGS += -D_OSF_SOURCE
 	SOCKLEN_T = int
-	NO_STRTOULL = YesPlease
+	NO_STRTOLL = YesPlease
 	NO_NSEC = YesPlease
 endif
 ifeq ($(uname_S),Linux)
@@ -890,7 +890,7 @@ ifeq ($(uname_S),SunOS)
 		NO_UNSETENV = YesPlease
 		NO_SETENV = YesPlease
 		NO_STRLCPY = YesPlease
-		NO_STRTOUMAX = YesPlease
+		NO_STRTOMAX = YesPlease
 		GIT_TEST_CMP = cmp
 	endif
 	ifeq ($(uname_R),5.7)
@@ -900,19 +900,19 @@ ifeq ($(uname_S),SunOS)
 		NO_UNSETENV = YesPlease
 		NO_SETENV = YesPlease
 		NO_STRLCPY = YesPlease
-		NO_STRTOUMAX = YesPlease
+		NO_STRTOMAX = YesPlease
 		GIT_TEST_CMP = cmp
 	endif
 	ifeq ($(uname_R),5.8)
 		NO_UNSETENV = YesPlease
 		NO_SETENV = YesPlease
-		NO_STRTOUMAX = YesPlease
+		NO_STRTOMAX = YesPlease
 		GIT_TEST_CMP = cmp
 	endif
 	ifeq ($(uname_R),5.9)
 		NO_UNSETENV = YesPlease
 		NO_SETENV = YesPlease
-		NO_STRTOUMAX = YesPlease
+		NO_STRTOMAX = YesPlease
 		GIT_TEST_CMP = cmp
 	endif
 	INSTALL = /usr/ucb/install
@@ -954,7 +954,7 @@ ifeq ($(uname_S),FreeBSD)
 	ifeq ($(shell expr "$(uname_R)" : '4\.'),2)
 		PTHREAD_LIBS = -pthread
 		NO_UINTMAX_T = YesPlease
-		NO_STRTOUMAX = YesPlease
+		NO_STRTOMAX = YesPlease
 	endif
 	PYTHON_PATH = /usr/local/bin/python
 	HAVE_PATHS_H = YesPlease
@@ -1092,8 +1092,8 @@ ifeq ($(uname_S),Windows)
 	NO_MEMMEM = YesPlease
 	# NEEDS_LIBICONV = YesPlease
 	NO_ICONV = YesPlease
-	NO_STRTOUMAX = YesPlease
-	NO_STRTOULL = YesPlease
+	NO_STRTOMAX = YesPlease
+	NO_STRTOLL = YesPlease
 	NO_MKDTEMP = YesPlease
 	NO_MKSTEMPS = YesPlease
 	SNPRINTF_RETURNS_BOGUS = YesPlease
@@ -1139,7 +1139,7 @@ ifeq ($(uname_S),Interix)
 	NO_IPV6 = YesPlease
 	NO_MEMMEM = YesPlease
 	NO_MKDTEMP = YesPlease
-	NO_STRTOUMAX = YesPlease
+	NO_STRTOMAX = YesPlease
 	NO_NSEC = YesPlease
 	NO_MKSTEMPS = YesPlease
 	ifeq ($(uname_R),3.5)
@@ -1184,7 +1184,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_MEMMEM = YesPlease
 	NEEDS_LIBICONV = YesPlease
 	OLD_ICONV = YesPlease
-	NO_STRTOUMAX = YesPlease
+	NO_STRTOMAX = YesPlease
 	NO_MKDTEMP = YesPlease
 	NO_MKSTEMPS = YesPlease
 	NO_SVN_TESTS = YesPlease
@@ -1440,12 +1440,12 @@ ifdef NO_STRLCPY
 	COMPAT_CFLAGS += -DNO_STRLCPY
 	COMPAT_OBJS += compat/strlcpy.o
 endif
-ifdef NO_STRTOUMAX
-	COMPAT_CFLAGS += -DNO_STRTOUMAX
-	COMPAT_OBJS += compat/strtoumax.o
+ifdef NO_STRTOMAX
+	COMPAT_CFLAGS += -DNO_STRTOMAX
+	COMPAT_OBJS += compat/strtoumax.o compat/strtoimax.o
 endif
-ifdef NO_STRTOULL
-	COMPAT_CFLAGS += -DNO_STRTOULL
+ifdef NO_STRTOLL
+	COMPAT_CFLAGS += -DNO_STRTOLL
 endif
 ifdef NO_STRTOK_R
 	COMPAT_CFLAGS += -DNO_STRTOK_R
diff --git a/compat/strtoimax.c b/compat/strtoimax.c
new file mode 100644
index 0000000..fca07a3
--- /dev/null
+++ b/compat/strtoimax.c
@@ -0,0 +1,10 @@
+#include "../git-compat-util.h"
+
+intmax_t gitstrtoimax (const char *nptr, char **endptr, int base)
+{
+#if defined(NO_STRTOLL)
+	return strtol(nptr, endptr, base);
+#else
+	return strtoll(nptr, endptr, base);
+#endif
+}
diff --git a/compat/strtoumax.c b/compat/strtoumax.c
index 5541353..7feedfd 100644
--- a/compat/strtoumax.c
+++ b/compat/strtoumax.c
@@ -2,7 +2,7 @@
 
 uintmax_t gitstrtoumax (const char *nptr, char **endptr, int base)
 {
-#if defined(NO_STRTOULL)
+#if defined(NO_STRTOLL)
 	return strtoul(nptr, endptr, base);
 #else
 	return strtoull(nptr, endptr, base);
-- 
1.7.6.1.139.gcb612

[PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes.

[Nix]

The config options core.packedGitWindowSize, core.packedGitLimit,
core.deltaBaseCacheLimit, core.bigFileThreshold, pack.windowMemory and
pack.packSizeLimit all claim to support suffixes up to and including
'g'.  This implies that they should accept sizes >=2G on 64-bit
systems: certainly, specifying a size of 3g should not silently be
translated to zero or transformed into a large negative value due to
integer overflow. However, due to use of git_config_int() rather than
git_config_ulong(), that is exactly what happens:

% git config core.bigFileThreshold 2g
% git gc --aggressive # with extra debugging code to print out
                      # core.bigfilethreshold after parsing
bigfilethreshold: -2147483648
[...]

This is probably irrelevant for core.deltaBaseCacheLimit, but is
problematic for the other values. (It is particularly problematic for
core.packedGitLimit, which can't even be set to its default value in
the config file due to this bug.)

This fixes things for 32-bit platforms as well. They get the usual bad
config error if an overlarge value is specified, e.g.:

fatal: bad config value for 'core.bigfilethreshold' in /home/nix/.gitconfig

32-bit platforms with no type larger than 'long' cannot detect this
case and will continue to silently misbehave, but the misbehaviour
will be somewhat different and more useful, since bigFileThreshold was
also being mistakenly treated as a signed value when it should have
been unsigned.

Signed-off-by: Nick Alcock <nix@esperi.org.uk>
---
 config.c |   26 ++++++++++++++++----------
 1 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/config.c b/config.c
index 4183f80..b19df66 100644
--- a/config.c
+++ b/config.c
@@ -333,7 +333,7 @@ static int git_parse_file(config_fn_t fn, void *data)
 	die("bad config file line %d in %s", cf->linenr, cf->name);
 }
 
-static int parse_unit_factor(const char *end, unsigned long *val)
+static int parse_unit_factor(const char *end, uintmax_t *val)
 {
 	if (!*end)
 		return 1;
@@ -356,11 +356,14 @@ static int git_parse_long(const char *value, long *ret)
 {
 	if (value && *value) {
 		char *end;
-		long val = strtol(value, &end, 0);
-		unsigned long factor = 1;
+		intmax_t val = strtoimax(value, &end, 0);
+		uintmax_t factor = 1;
 		if (!parse_unit_factor(end, &factor))
 			return 0;
-		*ret = val * factor;
+		val *= factor;
+		if (val > maximum_signed_value_of_type(long))
+			return 0;
+		*ret = val;
 		return 1;
 	}
 	return 0;
@@ -370,9 +373,11 @@ int git_parse_ulong(const char *value, unsigned long *ret)
 {
 	if (value && *value) {
 		char *end;
-		unsigned long val = strtoul(value, &end, 0);
+		uintmax_t val = strtoumax(value, &end, 0);
 		if (!parse_unit_factor(end, &val))
 			return 0;
+		if (val > maximum_unsigned_value_of_type(long))
+			return 0;
 		*ret = val;
 		return 1;
 	}
@@ -391,6 +396,8 @@ int git_config_int(const char *name, const char *value)
 	long ret = 0;
 	if (!git_parse_long(value, &ret))
 		die_bad_config(name);
+	if (ret > maximum_signed_value_of_type(int))
+		die_bad_config(name);
 	return ret;
 }
 
@@ -550,7 +557,7 @@ static int git_default_core_config(const char *var, const char *value)
 
 	if (!strcmp(var, "core.packedgitwindowsize")) {
 		int pgsz_x2 = getpagesize() * 2;
-		packed_git_window_size = git_config_int(var, value);
+		packed_git_window_size = git_config_ulong(var, value);
 
 		/* This value must be multiple of (pagesize * 2) */
 		packed_git_window_size /= pgsz_x2;
@@ -561,18 +568,17 @@ static int git_default_core_config(const char *var, const char *value)
 	}
 
 	if (!strcmp(var, "core.bigfilethreshold")) {
-		long n = git_config_int(var, value);
-		big_file_threshold = 0 < n ? n : 0;
+		big_file_threshold = git_config_ulong(var, value);
 		return 0;
 	}
 
 	if (!strcmp(var, "core.packedgitlimit")) {
-		packed_git_limit = git_config_int(var, value);
+		packed_git_limit = git_config_ulong(var, value);
 		return 0;
 	}
 
 	if (!strcmp(var, "core.deltabasecachelimit")) {
-		delta_base_cache_limit = git_config_int(var, value);
+		delta_base_cache_limit = git_config_ulong(var, value);
 		return 0;
 	}
 
-- 
1.7.6.1.139.gcb612

Re: [PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes.

[Sverre Rabbelier]

Heya,

On Mon, Sep 5, 2011 at 13:45, Nix <nix@esperi.org.uk> wrote:
> 32-bit platforms with no type larger than 'long' cannot detect this
> case and will continue to silently misbehave, but the misbehaviour
> will be somewhat different and more useful, since bigFileThreshold was
> also being mistakenly treated as a signed value when it should have
> been unsigned.

Is it not possible to detect that the target value won't fit in the
max size of an int when parsing the config value?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes.

[Nix]

On 5 Sep 2011, Sverre Rabbelier said:

> Heya,
>
> On Mon, Sep 5, 2011 at 13:45, Nix <nix@esperi.org.uk> wrote:
>> 32-bit platforms with no type larger than 'long' cannot detect this
>> case and will continue to silently misbehave, but the misbehaviour
>> will be somewhat different and more useful, since bigFileThreshold was
>> also being mistakenly treated as a signed value when it should have
>> been unsigned.
>
> Is it not possible to detect that the target value won't fit in the
> max size of an int when parsing the config value?

Well, we're parsing longs, not ints. If sizeof(long)>sizeof(int), or we
have long long and sizeof(long long)>sizeof(int), then we can always
detect overflows when saving into the appropriate type: but if we don't
have long long, or if we have neither strto(u)ll() nor strto[ui]max(),
we could only detect overflow by looking at the raw text string and
checking it by hand to see if it would fit. I judged this pointless
extra complexity for a very rare edge case (machines with neither
strot(u)ll() nor strto[ui]max() are generally quite old and people
aren't going to be specifying sizes in gigabytes on such machines
anyway.)

-- 
NULL && (void)

Re: [PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes.

[Clemens Buchacher]

On Mon, Sep 05, 2011 at 02:56:10PM +0100, Nix wrote:
> 
> Well, we're parsing longs, not ints. If sizeof(long)>sizeof(int), or we
> have long long and sizeof(long long)>sizeof(int), then we can always
> detect overflows when saving into the appropriate type: but if we don't
> have long long, or if we have neither strto(u)ll() nor strto[ui]max(),
> we could only detect overflow by looking at the raw text string and
> checking it by hand to see if it would fit. I judged this pointless
> extra complexity for a very rare edge case (machines with neither
> strot(u)ll() nor strto[ui]max() are generally quite old and people
> aren't going to be specifying sizes in gigabytes on such machines
> anyway.)

Is this also true for Windows and other platforms?

And I don't think it's about whether or not people are likely to
specify sizes in gigabytes on old machines. People are bound to
blindly copy configuration files from one machine to another. In
any case, my expectation would be for the configuration options to
do what I tell them, or error out if they do not make sense.

Clemens

Re: [PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes.

[Nix]

On 6 Sep 2011, Clemens Buchacher uttered the following:

> On Mon, Sep 05, 2011 at 02:56:10PM +0100, Nix wrote:
>> 
>> Well, we're parsing longs, not ints. If sizeof(long)>sizeof(int), or we
>> have long long and sizeof(long long)>sizeof(int), then we can always
>> detect overflows when saving into the appropriate type: but if we don't
>> have long long, or if we have neither strto(u)ll() nor strto[ui]max(),
>> we could only detect overflow by looking at the raw text string and
>> checking it by hand to see if it would fit. I judged this pointless
>> extra complexity for a very rare edge case (machines with neither
>> strot(u)ll() nor strto[ui]max() are generally quite old and people
>> aren't going to be specifying sizes in gigabytes on such machines
>> anyway.)
>
> Is this also true for Windows and other platforms?

There, uintmax_t is 'long long' and is longer than 'long', let alone
'int', so this holds there too, or should.

> And I don't think it's about whether or not people are likely to
> specify sizes in gigabytes on old machines. People are bound to
> blindly copy configuration files from one machine to another. In
> any case, my expectation would be for the configuration options to
> do what I tell them, or error out if they do not make sense.

Yeah, and we do that whenever practically possible: but fixing
this for the case that int/long is the largest available type
(which among other things implies that we're not using GCC or
any other C99 compiler or any of the myriad C89 compilers that
implmented 'long long') amounts to writing our own strtol()
specifically for this one case, to see if the parsed number is
too long. And that is probably a maintenance burden too far.

The failure mode if you put a huge number in on such a platform is
better than it used to be, too, especially for core.bigfilethreshold. We
used to get a negative number that was latched to zero, which then
disabled compression entirely (I had an 83Mb pack turn itself into an
837Mb one when that happened). Now we get a number that, while positive,
is less positive than we expect, but still likely up in the hundreds of
millions.

-- 
NULL && (void)

Re: [PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes.

[Johannes Sixt]

Am 9/6/2011 11:13, schrieb Nix:
> ... amounts to writing our own strtol()
> specifically for this one case, to see if the parsed number is
> too long.

Why so? strtol() can report overflow:

RETURN VALUE

    ...
    If the correct value is outside the range of representable values,
{LONG_MIN}, {LONG_MAX}, {LLONG_MIN}, or {LLONG_MAX} shall be returned
(according to the sign of the value), and errno set to [ERANGE].

-- Hannes

Re: [PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes.

[Nix]

On 6 Sep 2011, Johannes Sixt verbalised:

> Why so? strtol() can report overflow:

... it can?!

>     ...
>     If the correct value is outside the range of representable values,
> {LONG_MIN}, {LONG_MAX}, {LLONG_MIN}, or {LLONG_MAX} shall be returned
> (according to the sign of the value), and errno set to [ERANGE].

I've been using it for longer than I care to imagine and I've never once
noticed that.

OK, I'll add range checking support then! following which we can detect
config value overflow on the most pathetic platform imaginable, except
that such a platform would probably not bother to set ERANGE properly ;}

Fixed patch following later today ripping out the STRTOMAX renaming and
adding proper range checking.

-- 
NULL && (void)

Re: [PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes.

[Johannes Sixt]

Am 9/6/2011 12:25, schrieb Nix:
> OK, I'll add range checking support then!

Don't forget to follow this advice noted on the POSIX page
(http://pubs.opengroup.org/onlinepubs/9699919799/functions/strtol.html):

Since 0, {LONG_MIN} or {LLONG_MIN}, and {LONG_MAX} or {LLONG_MAX} are
returned on error and are also valid returns on success, an application
wishing to check for error situations should set errno to 0, then call
strtol() or strtoll(), then check errno.

-- Hannes

Re: [PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes.

[Nix]

On 6 Sep 2011, Johannes Sixt stated:

> Am 9/6/2011 12:25, schrieb Nix:
>> OK, I'll add range checking support then!
>
> Don't forget to follow this advice noted on the POSIX page
> (http://pubs.opengroup.org/onlinepubs/9699919799/functions/strtol.html):
>
> Since 0, {LONG_MIN} or {LLONG_MIN}, and {LONG_MAX} or {LLONG_MAX} are
> returned on error and are also valid returns on success, an application
> wishing to check for error situations should set errno to 0, then call
> strtol() or strtoll(), then check errno.

Oh, I've fallen into *that* trap before with math functions setting
ERANGE/EDOM. Thanks for the reminder :)

-- 
NULL && (void)

Re: [PATCH 1/2] Add strtoimax() compatibility function.

[Junio C Hamano]

Nix <nix@esperi.org.uk> writes:

> Since systems that omit strtoumax() will likely omit strtomax() too,
> and likewise for strtoull() and strtoll(), we also adjust the
> compatibility #defines from NO_STRTOUMAX to NO_STRTOMAX and from
> NO_STRTOULL to NO_STRTOLL, and have them cover both the signed and
> unsigned functions.

What would happen to people who know their systems lack strtoumax and have
happily using NO_STRTOUMAX in their config.mak already? Do their build
suddenly start breaking after this patch is applied and they all have to
adjust to the new name?

Even though "no strtoumax() likely means no strtoimax()" may be a good
heuristics, I am not sure what we would gain by renaming these Makefile
variables. Can't you get the same effect by making existing NO_STRTOUMAX
imply not having strtoimax(), and if you did so, wouldn't it be much less
likely that you would break existing people's build?

Re: [PATCH 1/2] Add strtoimax() compatibility function.

[Nix]

On 6 Sep 2011, Junio C. Hamano spake thusly:

> Nix <nix@esperi.org.uk> writes:
>
>> Since systems that omit strtoumax() will likely omit strtomax() too,
>> and likewise for strtoull() and strtoll(), we also adjust the
>> compatibility #defines from NO_STRTOUMAX to NO_STRTOMAX and from
>> NO_STRTOULL to NO_STRTOLL, and have them cover both the signed and
>> unsigned functions.
>
> What would happen to people who know their systems lack strtoumax and have
> happily using NO_STRTOUMAX in their config.mak already? Do their build
> suddenly start breaking after this patch is applied and they all have to
> adjust to the new name?

Uh. Yeah. Oops.

> Even though "no strtoumax() likely means no strtoimax()" may be a good
> heuristics, I am not sure what we would gain by renaming these Makefile
> variables. Can't you get the same effect by making existing NO_STRTOUMAX
> imply not having strtoimax(), and if you did so, wouldn't it be much less
> likely that you would break existing people's build?

Yes, but I thought that might be too confusing (and having four
variables for this one case seemed ridiculous). I'm happy to rename it
back.

-- 
NULL && (void)

[PATCH 0/2] Support sizes >=2G in various config options, v2

[Nick Alcock]

New in this version:

 - overflow detection, as suggested by Johannes Sixt (on 32-bit
   platforms too).
 - no renaming of NO_STRTOUMAX nor NO_STRTOULL.

I think this covers all the bases, including detection of configuration
values that overflow signed but not unsigned type only after
factor-application (as '3g' would on a 32-bit Linux box).

No new git testsuite failures. (No tests either, because I can't think
of one that will reliably induce overflow on a 64-bit box without being
totally ludicrous.)

Nick Alcock (2):
  Add strtoimax() compatibility function.
  Support sizes >=2G in various config options accepting 'g' sizes.

 Makefile           |    6 +++---
 compat/strtoimax.c |   10 ++++++++++
 config.c           |   43 +++++++++++++++++++++++++++++++++----------
 3 files changed, 46 insertions(+), 13 deletions(-)
 create mode 100644 compat/strtoimax.c

-- 
1.7.6.1.138.g03ab.dirty

[PATCH 1/2] Add strtoimax() compatibility function.

[Nick Alcock]

Since systems that omit strtoumax() will likely omit strtomax() too,
and likewise for strtoull() and strtoll(), we arrange for the
compatibility #defines NO_STRTOUMAX and NO_STRTOULL to cover both
the signed and unsigned functions. (We cannot change their names
without breaking existing makefile configurations.)

Signed-off-by: Nick Alcock <nix@esperi.org.uk>
---
 Makefile           |    6 +++---
 compat/strtoimax.c |   10 ++++++++++
 2 files changed, 13 insertions(+), 3 deletions(-)
 create mode 100644 compat/strtoimax.c

diff --git a/Makefile b/Makefile
index 303a8df..a1f7e34 100644
--- a/Makefile
+++ b/Makefile
@@ -58,8 +58,8 @@ include /home/compiler/.configure/site.mk
 #
 # Define NO_STRLCPY if you don't have strlcpy.
 #
-# Define NO_STRTOUMAX if you don't have strtoumax in the C library.
-# If your compiler also does not support long long or does not have
+# Define NO_STRTOUMAX if you don't have both strtoimax and strtoumax in the
+# C library. If your compiler also does not support long long or does not have
 # strtoull, define NO_STRTOULL.
 #
 # Define NO_SETENV if you don't have setenv in the C library.
@@ -1459,7 +1459,7 @@ ifdef NO_STRLCPY
 endif
 ifdef NO_STRTOUMAX
 	COMPAT_CFLAGS += -DNO_STRTOUMAX
-	COMPAT_OBJS += compat/strtoumax.o
+	COMPAT_OBJS += compat/strtoumax.o compat/strtoimax.o
 endif
 ifdef NO_STRTOULL
 	COMPAT_CFLAGS += -DNO_STRTOULL
diff --git a/compat/strtoimax.c b/compat/strtoimax.c
new file mode 100644
index 0000000..ac09ed8
--- /dev/null
+++ b/compat/strtoimax.c
@@ -0,0 +1,10 @@
+#include "../git-compat-util.h"
+
+intmax_t gitstrtoimax (const char *nptr, char **endptr, int base)
+{
+#if defined(NO_STRTOULL)
+	return strtol(nptr, endptr, base);
+#else
+	return strtoll(nptr, endptr, base);
+#endif
+}
-- 
1.7.6.1.138.g03ab.dirty