From 7192777d226c5b8653c65112a309c2ebd3d6d5c5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 8 Sep 2013 04:29:27 -0400 Subject: config: factor out integer parsing from range checks When we are parsing integers for config, we use an intmax_t (or uintmax_t) internally, and then check against the size of our result type at the end. We can parameterize the maximum representable value, which will let us re-use the parsing code for a variety of range checks. Unfortunately, we cannot combine the signed and unsigned parsing functions easily, as we have to rely on the signed and unsigned C types internally. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/config.c b/config.c index e13a7b6..19e8229 100644 --- a/config.c +++ b/config.c @@ -468,7 +468,7 @@ static int parse_unit_factor(const char *end, uintmax_t *val) return 0; } -static int git_parse_long(const char *value, long *ret) +static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max) { if (value && *value) { char *end; @@ -484,8 +484,7 @@ static int git_parse_long(const char *value, long *ret) return 0; uval = abs(val); uval *= factor; - if ((uval > maximum_signed_value_of_type(long)) || - (abs(val) > uval)) + if (uval > max || abs(val) > uval) return 0; val *= factor; *ret = val; @@ -494,7 +493,7 @@ static int git_parse_long(const char *value, long *ret) return 0; } -int git_parse_ulong(const char *value, unsigned long *ret) +int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max) { if (value && *value) { char *end; @@ -508,8 +507,7 @@ int git_parse_ulong(const char *value, unsigned long *ret) oldval = val; if (!parse_unit_factor(end, &val)) return 0; - if ((val > maximum_unsigned_value_of_type(long)) || - (oldval > val)) + if (val > max || oldval > val) return 0; *ret = val; return 1; @@ -517,6 +515,24 @@ int git_parse_ulong(const char *value, unsigned long *ret) return 0; } +static int git_parse_long(const char *value, long *ret) +{ + intmax_t tmp; + if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(long))) + return 0; + *ret = tmp; + return 1; +} + +int git_parse_ulong(const char *value, unsigned long *ret) +{ + uintmax_t tmp; + if (!git_parse_unsigned(value, &tmp, maximum_unsigned_value_of_type(long))) + return 0; + *ret = tmp; + return 1; +} + static void die_bad_config(const char *name) { if (cf && cf->name) -- cgit v0.10.2-6-g49f6 From 42d194e95870a9eeea192789226b7d51b1c90c96 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 8 Sep 2013 04:33:08 -0400 Subject: config: properly range-check integer values When we look at a config value as an integer using the git_config_int function, we carefully range-check the value we get and complain if it is out of our range. But the range we compare to is that of a "long", which we then cast to an "int" in the function's return value. This means that on systems where "int" and "long" have different sizes (e.g., LP64 systems), we may pass the range check, but then return nonsense by truncating the value as we cast it to an int. We can solve this by converting git_parse_long into git_parse_int, and range-checking the "int" range. Nobody actually cared that we used a "long" internally, since the result was truncated anyway. And the only other caller of git_parse_long is git_config_maybe_bool, which should be fine to just use int (though we will now forbid out-of-range nonsense like setting "merge.ff" to "10g" to mean "true", which is probably a good thing). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/config.c b/config.c index 19e8229..d3f71b2 100644 --- a/config.c +++ b/config.c @@ -515,10 +515,10 @@ int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max) return 0; } -static int git_parse_long(const char *value, long *ret) +static int git_parse_int(const char *value, int *ret) { intmax_t tmp; - if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(long))) + if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(int))) return 0; *ret = tmp; return 1; @@ -542,8 +542,8 @@ static void die_bad_config(const char *name) int git_config_int(const char *name, const char *value) { - long ret = 0; - if (!git_parse_long(value, &ret)) + int ret; + if (!git_parse_int(value, &ret)) die_bad_config(name); return ret; } @@ -575,10 +575,10 @@ static int git_config_maybe_bool_text(const char *name, const char *value) int git_config_maybe_bool(const char *name, const char *value) { - long v = git_config_maybe_bool_text(name, value); + int v = git_config_maybe_bool_text(name, value); if (0 <= v) return v; - if (git_parse_long(value, &v)) + if (git_parse_int(value, &v)) return !!v; return -1; } -- cgit v0.10.2-6-g49f6 From 33fdd77e2b8ece3490982f9a35c8669d16879ba8 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 8 Sep 2013 04:36:42 -0400 Subject: config: set errno in numeric git_parse_* functions When we are parsing an integer or unsigned long, we use the strto*max functions, which properly set errno to ERANGE if we get a large value. However, we also do further range checks after applying our multiplication factor, but do not set ERANGE. This means that a caller cannot tell if an error was caused by ERANGE or if the input was simply not a valid number. This patch teaches git_parse_signed and git_parse_unsigned to set ERANGE for range errors, and EINVAL for other errors, so that the caller can reliably tell these cases apart. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/config.c b/config.c index d3f71b2..9a42ad2 100644 --- a/config.c +++ b/config.c @@ -480,16 +480,21 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max) val = strtoimax(value, &end, 0); if (errno == ERANGE) return 0; - if (!parse_unit_factor(end, &factor)) + if (!parse_unit_factor(end, &factor)) { + errno = EINVAL; return 0; + } uval = abs(val); uval *= factor; - if (uval > max || abs(val) > uval) + if (uval > max || abs(val) > uval) { + errno = ERANGE; return 0; + } val *= factor; *ret = val; return 1; } + errno = EINVAL; return 0; } @@ -505,13 +510,18 @@ int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max) if (errno == ERANGE) return 0; oldval = val; - if (!parse_unit_factor(end, &val)) + if (!parse_unit_factor(end, &val)) { + errno = EINVAL; return 0; - if (val > max || oldval > val) + } + if (val > max || oldval > val) { + errno = ERANGE; return 0; + } *ret = val; return 1; } + errno = EINVAL; return 0; } -- cgit v0.10.2-6-g49f6 From 2f666581bbc1895cf66a7007fb222026b299deb9 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 8 Sep 2013 04:38:22 -0400 Subject: config: make numeric parsing errors more clear If we try to parse an integer config argument and get a number outside of the representable range, we die with the cryptic message: "bad config value for '%s'". We can improve two things: 1. Show the value that produced the error (e.g., bad config value '3g' for 'foo.bar'). 2. Mention the reason the value was rejected (e.g., "invalid unit" versus "out of range"). A few tests need to be updated with the new output, but that should not be representative of real-world breakage, as scripts should not be depending on the exact text of our stderr output, which is subject to i18n anyway. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/config.c b/config.c index 9a42ad2..4be6c7b 100644 --- a/config.c +++ b/config.c @@ -543,18 +543,25 @@ int git_parse_ulong(const char *value, unsigned long *ret) return 1; } -static void die_bad_config(const char *name) +static void die_bad_number(const char *name, const char *value) { + const char *reason = errno == ERANGE ? + "out of range" : + "invalid unit"; + if (!value) + value = ""; + if (cf && cf->name) - die("bad config value for '%s' in %s", name, cf->name); - die("bad config value for '%s'", name); + die("bad numeric config value '%s' for '%s' in %s: %s", + value, name, cf->name, reason); + die("bad numeric config value '%s' for '%s': %s", value, name, reason); } int git_config_int(const char *name, const char *value) { int ret; if (!git_parse_int(value, &ret)) - die_bad_config(name); + die_bad_number(name, value); return ret; } @@ -562,7 +569,7 @@ unsigned long git_config_ulong(const char *name, const char *value) { unsigned long ret; if (!git_parse_ulong(value, &ret)) - die_bad_config(name); + die_bad_number(name, value); return ret; } diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index c4a7d84..20aee6e 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -657,11 +657,11 @@ test_expect_success 'invalid unit' ' echo 1auto >expect && git config aninvalid.unit >actual && test_cmp expect actual && - cat > expect <<-\EOF - fatal: bad config value for '\''aninvalid.unit'\'' in .git/config + cat >expect <<-\EOF + fatal: bad numeric config value '\''1auto'\'' for '\''aninvalid.unit'\'' in .git/config: invalid unit EOF test_must_fail git config --int --get aninvalid.unit 2>actual && - test_cmp actual expect + test_i18ncmp expect actual ' cat > expect << EOF diff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh index 97172b4..cd04543 100755 --- a/t/t4055-diff-context.sh +++ b/t/t4055-diff-context.sh @@ -73,7 +73,7 @@ test_expect_success 'plumbing not affected' ' test_expect_success 'non-integer config parsing' ' git config diff.context no && test_must_fail git diff 2>output && - test_i18ngrep "bad config value" output + test_i18ngrep "bad numeric config value" output ' test_expect_success 'negative integer config parsing' ' -- cgit v0.10.2-6-g49f6 From 00160242770aea137ec7154a8e8406feef733926 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 8 Sep 2013 04:40:02 -0400 Subject: git-config: always treat --int as 64-bit internally When you run "git config --int", the maximum size of integer you get depends on how git was compiled, and what it considers to be an "int". This is almost useful, because your scripts calling "git config" will behave similarly to git internally. But relying on this is dubious; you have to actually know how git treats each value internally (e.g., int versus unsigned long), which is not documented and is subject to change. And even if you know it is "unsigned long", we do not have a git-config option to match that behavior. Furthermore, you may simply be asking git to store a value on your behalf (e.g., configuration for a hook). In that case, the relevant range check has nothing at all to do with git, but rather with whatever scripting tools you are using (and git has no way of knowing what the appropriate range is there). Not only is the range check useless, but it is actively harmful, as there is no way at all for scripts to look at config variables with large values. For instance, one cannot reliably get the value of pack.packSizeLimit via git-config. On an LP64 system, git happily uses a 64-bit "unsigned long" internally to represent the value, but the script cannot read any value over 2G. Ideally, the "--int" option would simply represent an arbitrarily large integer. For practical purposes, however, a 64-bit integer is large enough, and is much easier to implement (and if somebody overflows it, we will still notice the problem, and not simply return garbage). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/builtin/config.c b/builtin/config.c index 4010c43..8b182d2 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -128,7 +128,8 @@ static int collect_config(const char *key_, const char *value_, void *cb) must_print_delim = 1; } if (types == TYPE_INT) - sprintf(value, "%d", git_config_int(key_, value_?value_:"")); + sprintf(value, "%"PRId64, + git_config_int64(key_, value_ ? value_ : "")); else if (types == TYPE_BOOL) vptr = git_config_bool(key_, value_) ? "true" : "false"; else if (types == TYPE_BOOL_OR_INT) { @@ -265,8 +266,8 @@ static char *normalize_value(const char *key, const char *value) else { normalized = xmalloc(64); if (types == TYPE_INT) { - int v = git_config_int(key, value); - sprintf(normalized, "%d", v); + int64_t v = git_config_int64(key, value); + sprintf(normalized, "%"PRId64, v); } else if (types == TYPE_BOOL) sprintf(normalized, "%s", diff --git a/cache.h b/cache.h index 85b544f..ac4525a 100644 --- a/cache.h +++ b/cache.h @@ -1190,6 +1190,7 @@ extern int git_config_with_options(config_fn_t fn, void *, extern int git_config_early(config_fn_t fn, void *, const char *repo_config); extern int git_parse_ulong(const char *, unsigned long *); extern int git_config_int(const char *, const char *); +extern int64_t git_config_int64(const char *, const char *); extern unsigned long git_config_ulong(const char *, const char *); extern int git_config_bool_or_int(const char *, const char *, int *); extern int git_config_bool(const char *, const char *); diff --git a/config.c b/config.c index 4be6c7b..3ffe134 100644 --- a/config.c +++ b/config.c @@ -534,6 +534,15 @@ static int git_parse_int(const char *value, int *ret) return 1; } +static int git_parse_int64(const char *value, int64_t *ret) +{ + intmax_t tmp; + if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(int64_t))) + return 0; + *ret = tmp; + return 1; +} + int git_parse_ulong(const char *value, unsigned long *ret) { uintmax_t tmp; @@ -565,6 +574,14 @@ int git_config_int(const char *name, const char *value) return ret; } +int64_t git_config_int64(const char *name, const char *value) +{ + int64_t ret; + if (!git_parse_int64(value, &ret)) + die_bad_number(name, value); + return ret; +} + unsigned long git_config_ulong(const char *name, const char *value) { unsigned long ret; diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index 20aee6e..b66c632 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -652,6 +652,13 @@ test_expect_success numbers ' test_cmp expect actual ' +test_expect_success '--int is at least 64 bits' ' + git config giga.watts 121g && + echo 129922760704 >expect && + git config --int --get giga.watts >actual && + test_cmp expect actual +' + test_expect_success 'invalid unit' ' git config aninvalid.unit "1auto" && echo 1auto >expect && -- cgit v0.10.2-6-g49f6