[PATCH 1/2] 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 there were no other callers
of git_parse_long.
Of the existing callers, all of them expect small-ish values
that are fine for an int, and should not be impacted
except when nonsense is fed to them. The one arguable
exception is http.lowSpeedLimit, which is given in bytes per
second, and is fed to curl as a long. However, nobody is
likely to consider 2GB/sec as low speed, and even if they
did, the new behavior is much better (it barfs and complains
rather than considering the value negative and silently
ignoring it).
Signed-off-by: Jeff King p...@peff.net
---
I added a test. It would not fail on existing 32-bit systems, but would
on existing LP64 systems. It will pass with the new code on both.
However, it will fail on ILP64 systems (because their int is large, and
can represent 3GB). I'm not sure if any such systems are in wide use
(SPARC64?), but we would want a prereq in that case, I guess. I'm
inclined to wait to see if it actually fails for anybody.
config.c | 12 ++--
t/t1300-repo-config.sh | 5 +
2 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/config.c b/config.c
index e13a7b6..9237d4b 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_int(const char *value, int *ret)
{
if (value *value) {
char *end;
@@ -484,7 +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)) ||
+ if ((uval maximum_signed_value_of_type(int)) ||
(abs(val) uval))
return 0;
val *= factor;
@@ -526,8 +526,8 @@ int git_config_int(const char *name, const char *value)
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;
}
@@ -559,10 +559,10 @@ int git_config_maybe_bool(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;
}
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index c4a7d84..3c5ec4b 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -652,6 +652,11 @@ test_expect_success numbers '
test_cmp expect actual
'
+test_expect_success 'out-of-range integers are detected' '
+ git config giga.crash 3g
+ test_must_fail git config --int giga.crash
+'
+
test_expect_success 'invalid unit' '
git config aninvalid.unit 1auto
echo 1auto expect
--
1.8.4.rc2.28.g6bb5f3f
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] config: properly range-check integer values
Jeff King wrote:
I added a test. It would not fail on existing 32-bit systems, but would
on existing LP64 systems. It will pass with the new code on both.
However, it will fail on ILP64 systems (because their int is large, and
can represent 3GB). I'm not sure if any such systems are in wide use
(SPARC64?), but we would want a prereq in that case, I guess. I'm
inclined to wait to see if it actually fails for anybody.
Yuck.
What will go wrong if git config --int starts returning numbers too
large to fit in an 'int'? That can already happen if git and a
command that uses it are built for different ABIs (e.g., ILP64 git,
32-bit custom tool that calls git).
It's possible that what the test should be checking for is either
returns a sane answer or fails (which would pass regardless of ABI).
Something like:
test_expect_success 'large integers do not confuse config --int' '
git config giga.crash 3g
test_might_fail git config --int giga.crash actual
echo 3221225472 expect
{
test_cmp expect actual ||
test_must_fail git config --int giga.crash
}
'
Sensible?
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] config: properly range-check integer values
On Tue, Aug 20, 2013 at 04:07:49PM -0700, Jonathan Nieder wrote:
I added a test. It would not fail on existing 32-bit systems, but would
on existing LP64 systems. It will pass with the new code on both.
However, it will fail on ILP64 systems (because their int is large, and
can represent 3GB). I'm not sure if any such systems are in wide use
(SPARC64?), but we would want a prereq in that case, I guess. I'm
inclined to wait to see if it actually fails for anybody.
Yuck.
What will go wrong if git config --int starts returning numbers too
large to fit in an 'int'? That can already happen if git and a
command that uses it are built for different ABIs (e.g., ILP64 git,
32-bit custom tool that calls git).
I'm not sure I understand your question. git config --int cannot
return numbers that do not fit in an int, since we use an int as an
intermediate representation type. The intent of the code is to
range-check the input and complain. But the code gets it wrong, and
sometimes truncates the value instead. So we continue to never show
anything that would not fit in an int, but now our range-check
correctly complains rather than truncating in some cases.
If you are worried about a 32-bit command calling an ILP64 git, then
nothing is made worse by this patch. We return the same range of values
as before.
Short of adding --int32, etc to git-config, I don't think git can be
responsible for this situation. It can only say This does not fit in my
internal representation, and I would barf on it. If you do not tell it
that your int is smaller, then it cannot say you would barf on it.
It's possible that what the test should be checking for is either
returns a sane answer or fails (which would pass regardless of ABI).
Something like:
test_expect_success 'large integers do not confuse config --int' '
git config giga.crash 3g
test_might_fail git config --int giga.crash actual
echo 3221225472 expect
{
test_cmp expect actual ||
test_must_fail git config --int giga.crash
}
'
Sensible?
Yes, that would cover an ILP64 system, though it very much muddies the
point of the test (which is to find a value that is represented by a
long, but not an int; such a value does not exist at all on ILP64).
Which is why I wondered about avoiding it with a prerequisite.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
