* [PATCH] config: don't segfault when given --path with a missing value @ 2012-11-14 4:50 Carlos Martín Nieto 2012-11-15 16:08 ` Jeff King 0 siblings, 1 reply; 6+ messages in thread From: Carlos Martín Nieto @ 2012-11-14 4:50 UTC (permalink / raw) To: git 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. Set the must_free_vptr flag depending on the return value of git_config_pathname so it's accurate. --- builtin/config.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index 442ccc2..60220d5 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -129,8 +129,7 @@ static int show_config(const char *key_, const char *value_, void *cb) else sprintf(value, "%d", v); } else if (types == TYPE_PATH) { - git_config_pathname(&vptr, key_, value_); - must_free_vptr = 1; + must_free_vptr = !git_config_pathname(&vptr, key_, value_); } else if (value_) { vptr = value_; } else { -- 1.8.0.316.g291341c ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] config: don't segfault when given --path with a missing value 2012-11-14 4:50 [PATCH] config: don't segfault when given --path with a missing value Carlos Martín Nieto @ 2012-11-15 16:08 ` Jeff King 2012-11-15 16:11 ` Jeff King 0 siblings, 1 reply; 6+ messages in thread From: Jeff King @ 2012-11-15 16:08 UTC (permalink / raw) To: Carlos Martín Nieto; +Cc: git 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] config: don't segfault when given --path with a missing value 2012-11-15 16:08 ` Jeff King @ 2012-11-15 16:11 ` Jeff King 2012-11-15 16:18 ` Jeff King 0 siblings, 1 reply; 6+ messages in thread From: Jeff King @ 2012-11-15 16:11 UTC (permalink / raw) To: Carlos Martín Nieto; +Cc: git 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] config: don't segfault when given --path with a missing value 2012-11-15 16:11 ` Jeff King @ 2012-11-15 16:18 ` Jeff King 2012-11-15 18:10 ` Carlos Martín Nieto 0 siblings, 1 reply; 6+ messages in thread From: Jeff King @ 2012-11-15 16:18 UTC (permalink / raw) To: Carlos Martín Nieto; +Cc: git 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] config: don't segfault when given --path with a missing value 2012-11-15 16:18 ` Jeff King @ 2012-11-15 18:10 ` Carlos Martín Nieto 2012-11-15 18:15 ` Jeff King 0 siblings, 1 reply; 6+ messages in thread From: Carlos Martín Nieto @ 2012-11-15 18:10 UTC (permalink / raw) To: Jeff King; +Cc: git 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 <cmn@elego.de> --- 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; 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. Do we have some equivalent of a Basically-writen-by line? builtin/config.c | 3 ++- t/t1300-repo-config.sh | 5 +++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/builtin/config.c b/builtin/config.c index 442ccc2..4dc5ffa 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -129,7 +129,8 @@ static int show_config(const char *key_, const char *value_, void *cb) else sprintf(value, "%d", v); } else if (types == TYPE_PATH) { - git_config_pathname(&vptr, key_, value_); + if (git_config_pathname(&vptr, key_, value_) < 0) + return -1; must_free_vptr = 1; } else if (value_) { vptr = value_; diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index a477453..17272e0 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" -- 1.8.0.316.g291341c ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] config: don't segfault when given --path with a missing value 2012-11-15 18:10 ` Carlos Martín Nieto @ 2012-11-15 18:15 ` Jeff King 0 siblings, 0 replies; 6+ messages in thread From: Jeff King @ 2012-11-15 18:15 UTC (permalink / raw) To: Carlos Martín Nieto; +Cc: git 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 <cmn@elego.de> Acked-by: Jeff King <peff@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-11-15 18:15 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-11-14 4:50 [PATCH] config: don't segfault when given --path with a missing value Carlos Martín Nieto 2012-11-15 16:08 ` Jeff King 2012-11-15 16:11 ` Jeff King 2012-11-15 16:18 ` Jeff King 2012-11-15 18:10 ` Carlos Martín Nieto 2012-11-15 18:15 ` Jeff King
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).