* [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).