Re: [PATCH] config: don't segfault when given --path with a missing value
On Tue, Nov 13, 2012 at 08:50:04PM -0800, Carlos Martín Nieto wrote:
When given a variable without a value, such as '[section] var' and
asking git-config to treat it as a path, git_config_pathname returns
an error and doesn't modify its output parameter. show_config assumes
that the call is always successful and sets a variable to indicate
that vptr should be freed. In case of an error however, trying to do
this will cause the program to be killed, as it's pointing to memory
in the stack.
Whoops.
Set the must_free_vptr flag depending on the return value of
git_config_pathname so it's accurate.
That is definitely the right thing to do. But do we also need to take
note of the error for later? After this code:
} else if (types == TYPE_PATH) {
- git_config_pathname(vptr, key_, value_);
- must_free_vptr = 1;
+ must_free_vptr = !git_config_pathname(vptr, key_, value_);
We don't have any clue that nothing got written into vptr. Which means
it still points at the stack buffer value, which contains
uninitialized bytes. We will later try to print it, thinking it has the
expanded path in it.
Do we need something like:
if (!git_config_pathname(vptr, key_, value_))
must_free_vptr = 1;
else
vptr = ;
?
-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
Re: [PATCH] config: don't segfault when given --path with a missing value
On Thu, Nov 15, 2012 at 08:08:49AM -0800, Jeff King wrote:
That is definitely the right thing to do. But do we also need to take
note of the error for later? After this code:
} else if (types == TYPE_PATH) {
- git_config_pathname(vptr, key_, value_);
- must_free_vptr = 1;
+ must_free_vptr = !git_config_pathname(vptr, key_, value_);
We don't have any clue that nothing got written into vptr. Which means
it still points at the stack buffer value, which contains
uninitialized bytes. We will later try to print it, thinking it has the
expanded path in it.
Do we need something like:
if (!git_config_pathname(vptr, key_, value_))
must_free_vptr = 1;
else
vptr = ;
Hmm, actually, we should probably propagate the error (I was thinking
for some reason this was in the listing code, but it is really about
getting a specific variable, and that variable does not have a sane
format. We'll already have printed the non-bool error, so we should
probably die. So more like:
if (git_config_pathname(vptr, key_, value_) 0)
return -1;
must_free_vptr = 1;
-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
Re: [PATCH] config: don't segfault when given --path with a missing value
On Thu, Nov 15, 2012 at 08:11:50AM -0800, Jeff King wrote: Hmm, actually, we should probably propagate the error (I was thinking for some reason this was in the listing code, but it is really about getting a specific variable, and that variable does not have a sane format. We'll already have printed the non-bool error, so we should probably die. So more like: if (git_config_pathname(vptr, key_, value_) 0) return -1; must_free_vptr = 1; You may want to squash in this test, which triggers your original problem, but also demonstrates the use of uninitialized memory (although you need to run under valgrind or similar to reliably notice it). diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index e127f35..7c4c372 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -803,6 +803,11 @@ test_expect_success NOT_MINGW 'get --path copes with unset $HOME' ' test_cmp expect result ' +test_expect_success 'get --path barfs on boolean variable' ' + echo [path]bool .git/config + test_must_fail git config --get --path path.bool +' + cat expect EOF [quote] leading = test -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
Re: [PATCH] config: don't segfault when given --path with a missing value
On Thu, Nov 15, 2012 at 10:10:01AM -0800, Carlos Martín Nieto wrote: When given a variable without a value, such as '[section] var' and asking git-config to treat it as a path, git_config_pathname returns an error and doesn't modify its output parameter. show_config assumes that the call is always successful and sets a variable to indicate that vptr should be freed. In case of an error however, trying to do this will cause the program to be killed, as it's pointing to memory in the stack. Detect the error and return immediately to avoid freeing or accessing the uninitialed memory in the stack. Signed-off-by: Carlos Martín Nieto c...@elego.de Acked-by: Jeff King p...@peff.net Yeah, that's more sensible. I didn't notice that the buffer never gets written to in this codepath, and the trying to print it out is silly when we know that there is nothing valid to print. Thanks for the review. I've included your test as well, which really makes all of this your code. Eh, I guess so. You did the hard part of finding it, though. ;) Do we have some equivalent of a Basically-writen-by line? Nothing structured. But I am comfortable enough with the number of times I am mentioned in git log already, so don't worry about it. -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
