* [PATCH 1/2] Add strtoimax() compatibility function.
@ 2011-09-05 11:45 Nix
2011-09-05 11:45 ` [PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes Nix
2011-09-06 6:19 ` [PATCH 1/2] Add strtoimax() compatibility function Junio C Hamano
0 siblings, 2 replies; 13+ messages in thread
From: Nix @ 2011-09-05 11:45 UTC (permalink / raw)
To: git; +Cc: 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes.
2011-09-05 11:45 [PATCH 1/2] Add strtoimax() compatibility function Nix
@ 2011-09-05 11:45 ` Nix
2011-09-05 13:49 ` Sverre Rabbelier
2011-09-06 6:19 ` [PATCH 1/2] Add strtoimax() compatibility function Junio C Hamano
1 sibling, 1 reply; 13+ messages in thread
From: Nix @ 2011-09-05 11:45 UTC (permalink / raw)
To: git; +Cc: 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes.
2011-09-05 11:45 ` [PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes Nix
@ 2011-09-05 13:49 ` Sverre Rabbelier
2011-09-05 13:56 ` Nix
0 siblings, 1 reply; 13+ messages in thread
From: Sverre Rabbelier @ 2011-09-05 13:49 UTC (permalink / raw)
To: Nix; +Cc: git
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes.
2011-09-05 13:49 ` Sverre Rabbelier
@ 2011-09-05 13:56 ` Nix
2011-09-06 7:44 ` Clemens Buchacher
0 siblings, 1 reply; 13+ messages in thread
From: Nix @ 2011-09-05 13:56 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: git
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)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes.
2011-09-05 13:56 ` Nix
@ 2011-09-06 7:44 ` Clemens Buchacher
2011-09-06 9:13 ` Nix
0 siblings, 1 reply; 13+ messages in thread
From: Clemens Buchacher @ 2011-09-06 7:44 UTC (permalink / raw)
To: Nix; +Cc: Sverre Rabbelier, git
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes.
2011-09-06 7:44 ` Clemens Buchacher
@ 2011-09-06 9:13 ` Nix
2011-09-06 10:22 ` Johannes Sixt
0 siblings, 1 reply; 13+ messages in thread
From: Nix @ 2011-09-06 9:13 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: Sverre Rabbelier, git
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)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes.
2011-09-06 9:13 ` Nix
@ 2011-09-06 10:22 ` Johannes Sixt
2011-09-06 10:25 ` Nix
0 siblings, 1 reply; 13+ messages in thread
From: Johannes Sixt @ 2011-09-06 10:22 UTC (permalink / raw)
To: Nix; +Cc: Clemens Buchacher, Sverre Rabbelier, git
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes.
2011-09-06 10:22 ` Johannes Sixt
@ 2011-09-06 10:25 ` Nix
2011-09-06 10:38 ` Johannes Sixt
0 siblings, 1 reply; 13+ messages in thread
From: Nix @ 2011-09-06 10:25 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Clemens Buchacher, Sverre Rabbelier, git
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)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes.
2011-09-06 10:25 ` Nix
@ 2011-09-06 10:38 ` Johannes Sixt
2011-09-06 11:17 ` Nix
0 siblings, 1 reply; 13+ messages in thread
From: Johannes Sixt @ 2011-09-06 10:38 UTC (permalink / raw)
To: Nix; +Cc: Clemens Buchacher, Sverre Rabbelier, git
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes.
2011-09-06 10:38 ` Johannes Sixt
@ 2011-09-06 11:17 ` Nix
0 siblings, 0 replies; 13+ messages in thread
From: Nix @ 2011-09-06 11:17 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Clemens Buchacher, Sverre Rabbelier, git
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)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] Add strtoimax() compatibility function.
2011-09-05 11:45 [PATCH 1/2] Add strtoimax() compatibility function Nix
2011-09-05 11:45 ` [PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes Nix
@ 2011-09-06 6:19 ` Junio C Hamano
2011-09-06 9:14 ` Nix
1 sibling, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2011-09-06 6:19 UTC (permalink / raw)
To: Nix; +Cc: git
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?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] Add strtoimax() compatibility function.
2011-09-06 6:19 ` [PATCH 1/2] Add strtoimax() compatibility function Junio C Hamano
@ 2011-09-06 9:14 ` Nix
0 siblings, 0 replies; 13+ messages in thread
From: Nix @ 2011-09-06 9:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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)
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 0/2] Support sizes >=2G in various config options, v2
@ 2011-11-02 15:46 Nick Alcock
2011-11-02 15:46 ` [PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes Nick Alcock
0 siblings, 1 reply; 13+ messages in thread
From: Nick Alcock @ 2011-11-02 15:46 UTC (permalink / raw)
To: git; +Cc: 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes.
2011-11-02 15:46 [PATCH 0/2] Support sizes >=2G in various config options, v2 Nick Alcock
@ 2011-11-02 15:46 ` Nick Alcock
0 siblings, 0 replies; 13+ messages in thread
From: Nick Alcock @ 2011-11-02 15:46 UTC (permalink / raw)
To: git; +Cc: Nick Alcock
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
This is detected in all cases, even if the 32-bit platform has no size
larger than 'long'. For signed integral configuration values, we also
detect the case where the value is too large for the signed type but
not the unsigned type.
Signed-off-by: Nick Alcock <nix@esperi.org.uk>
---
config.c | 41 +++++++++++++++++++++++++++++++----------
1 files changed, 31 insertions(+), 10 deletions(-)
diff --git a/config.c b/config.c
index edf9914..84ca156 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,23 @@ 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;
+ uintmax_t uval;
+ uintmax_t factor = 1;
+
+ errno = 0;
+ val = strtoimax(value, &end, 0);
+ if (errno == ERANGE)
+ return 0;
if (!parse_unit_factor(end, &factor))
return 0;
- *ret = val * factor;
+ uval = abs(val);
+ uval *= factor;
+ if ((uval > maximum_signed_value_of_type(long)) ||
+ (abs(val) > uval))
+ return 0;
+ val *= factor;
+ *ret = val;
return 1;
}
return 0;
@@ -370,9 +382,19 @@ 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;
+ uintmax_t oldval;
+
+ errno = 0;
+ val = strtoumax(value, &end, 0);
+ if (errno == ERANGE)
+ return 0;
+ oldval = val;
if (!parse_unit_factor(end, &val))
return 0;
+ if ((val > maximum_unsigned_value_of_type(long)) ||
+ (oldval > val))
+ return 0;
*ret = val;
return 1;
}
@@ -553,7 +575,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;
@@ -564,18 +586,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.138.g03ab.dirty
^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2011-11-02 16:25 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-05 11:45 [PATCH 1/2] Add strtoimax() compatibility function Nix
2011-09-05 11:45 ` [PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes Nix
2011-09-05 13:49 ` Sverre Rabbelier
2011-09-05 13:56 ` Nix
2011-09-06 7:44 ` Clemens Buchacher
2011-09-06 9:13 ` Nix
2011-09-06 10:22 ` Johannes Sixt
2011-09-06 10:25 ` Nix
2011-09-06 10:38 ` Johannes Sixt
2011-09-06 11:17 ` Nix
2011-09-06 6:19 ` [PATCH 1/2] Add strtoimax() compatibility function Junio C Hamano
2011-09-06 9:14 ` Nix
-- strict thread matches above, loose matches on Subject: below --
2011-11-02 15:46 [PATCH 0/2] Support sizes >=2G in various config options, v2 Nick Alcock
2011-11-02 15:46 ` [PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes Nick Alcock
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).