threads / patch / 19420

patchsetup_revisions(): do not access outside argv

Subject: [PATCH] setup_revisions(): do not access outside argv

## tl;dr

16 messages between May 20, 2009 and Jun 2, 2009. Diffs are folded; open one to read it.

replies: 15people: 7as markdown or json

Nguyễn Thái Ngọc Duy· May 20, 2009, 08:08 UTC · lore

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 revision.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Show changes to revision.c +2 −2

diff --git a/revision.c b/revision.c
index 18b7ebb..be1e307 100644
--- a/revision.c
+++ b/revision.c
@@ -1241,9 +1241,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 		if (strcmp(arg, "--"))
 			continue;
 		argv[i] = NULL;
-		argc = i;
-		if (argv[i + 1])
+		if (i + 1 < argc && argv[i + 1])
 			revs->prune_data = get_pathspec(revs->prefix, argv + i + 1);
+		argc = i;
 		seen_dashdash = 1;
 		break;
 	}

-- 
test

Johannes Sixt· May 20, 2009, 08:15 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH] setup_revisions(): do not access outside argv

Nguyễn Thái Ngọc Duy schrieb:

Show 21 quoted lines

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  revision.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/revision.c b/revision.c
> index 18b7ebb..be1e307 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1241,9 +1241,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
>  		if (strcmp(arg, "--"))
>  			continue;
>  		argv[i] = NULL;
> -		argc = i;
> -		if (argv[i + 1])
> +		if (i + 1 < argc && argv[i + 1])
>  			revs->prune_data = get_pathspec(revs->prefix, argv + i + 1);
> +		argc = i;
>  		seen_dashdash = 1;
>  		break;
>  	}

Why is this necessary? I'd expect that argv arrays have NULL at the end. If this is a bug, why did noboy notice this earlier? IOW, your commit message is really lacking justification.

-- Hannes

Nguyen Thai Ngoc Duy· May 20, 2009, 08:23 UTC · re: Johannes Sixt · lore

Re: [PATCH] setup_revisions(): do not access outside argv

2009/5/20 Johannes Sixt <j.sixt@viscovery.net>:

Show 24 quoted lines

> Nguyễn Thái Ngọc Duy schrieb:
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  revision.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/revision.c b/revision.c
>> index 18b7ebb..be1e307 100644
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -1241,9 +1241,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
>>               if (strcmp(arg, "--"))
>>                       continue;
>>               argv[i] = NULL;
>> -             argc = i;
>> -             if (argv[i + 1])
>> +             if (i + 1 < argc && argv[i + 1])
>>                       revs->prune_data = get_pathspec(revs->prefix, argv + i + 1);
>> +             argc = i;
>>               seen_dashdash = 1;
>>               break;
>>       }
>
> Why is this necessary? I'd expect that argv arrays have NULL at the end.

I have no idea. I hit this "bug" in my builtin-rebase.c and had that question too. But I grepped through and saw that at least verify_bundle() does not terminate argv with NULL. So I assume that setup_revisions() does not expect NULL at the end.

-- 
Duy

Junio C Hamano· May 21, 2009, 01:58 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCH] setup_revisions(): do not access outside argv

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

Show 30 quoted lines

> 2009/5/20 Johannes Sixt <j.sixt@viscovery.net>:
>> Nguyễn Thái Ngọc Duy schrieb:
>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>> ---
>>>  revision.c |    4 ++--
>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/revision.c b/revision.c
>>> index 18b7ebb..be1e307 100644
>>> --- a/revision.c
>>> +++ b/revision.c
>>> @@ -1241,9 +1241,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
>>>               if (strcmp(arg, "--"))
>>>                       continue;
>>>               argv[i] = NULL;
>>> -             argc = i;
>>> -             if (argv[i + 1])
>>> +             if (i + 1 < argc && argv[i + 1])
>>>                       revs->prune_data = get_pathspec(revs->prefix, argv + i + 1);
>>> +             argc = i;
>>>               seen_dashdash = 1;
>>>               break;
>>>       }
>>
>> Why is this necessary? I'd expect that argv arrays have NULL at the end.
>
> I have no idea. I hit this "bug" in my builtin-rebase.c and had that
> question too. But I grepped through and saw that
> at least verify_bundle() does not terminate argv with NULL. So I
> assume that setup_revisions() does not expect NULL at the end.

If a function takes (int ac, char **av), then people should be able to depend on the usual convention of

 (1) for any i < ac, av[i] is not NULL; and
 (2) av[ac] is NULL.

With your patch, a broken caller's wish is simply discarded and nobody will notice. Without your patch, at least you will know that the caller passed an inconsistent pair of ac and av to this function by seeing a coalmine canary segfault.

I would not mind a patch that adds an assertion that protects this function from broken callers, so that we can find them, but your patch makes me feel very uneasy.

Miles Bader· May 21, 2009, 02:38 UTC · re: Junio C Hamano · lore

Re: [PATCH] setup_revisions(): do not access outside argv

Junio C Hamano <gitster@pobox.com> writes:

Show 5 quoted lines

> If a function takes (int ac, char **av), then people should be able to
> depend on the usual convention of
>
>  (1) for any i < ac, av[i] is not NULL; and
>  (2) av[ac] is NULL.

Hmm, isn't potentially useful to be able to pass a sub-range (of a longer argv vector) to an ac/av function? In such a case, av[ac] may not be NULL.

-Miles

-- 
97% of everything is grunge

Nguyen Thai Ngoc Duy· May 21, 2009, 02:41 UTC · re: Junio C Hamano · lore

Re: [PATCH] setup_revisions(): do not access outside argv

On Thu, May 21, 2009 at 11:58 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 43 quoted lines

> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> 2009/5/20 Johannes Sixt <j.sixt@viscovery.net>:
>>> Nguyễn Thái Ngọc Duy schrieb:
>>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>>> ---
>>>>  revision.c |    4 ++--
>>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/revision.c b/revision.c
>>>> index 18b7ebb..be1e307 100644
>>>> --- a/revision.c
>>>> +++ b/revision.c
>>>> @@ -1241,9 +1241,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
>>>>               if (strcmp(arg, "--"))
>>>>                       continue;
>>>>               argv[i] = NULL;
>>>> -             argc = i;
>>>> -             if (argv[i + 1])
>>>> +             if (i + 1 < argc && argv[i + 1])
>>>>                       revs->prune_data = get_pathspec(revs->prefix, argv + i + 1);
>>>> +             argc = i;
>>>>               seen_dashdash = 1;
>>>>               break;
>>>>       }
>>>
>>> Why is this necessary? I'd expect that argv arrays have NULL at the end.
>>
>> I have no idea. I hit this "bug" in my builtin-rebase.c and had that
>> question too. But I grepped through and saw that
>> at least verify_bundle() does not terminate argv with NULL. So I
>> assume that setup_revisions() does not expect NULL at the end.
>
> If a function takes (int ac, char **av), then people should be able to
> depend on the usual convention of
>
>  (1) for any i < ac, av[i] is not NULL; and
>  (2) av[ac] is NULL.
>
> With your patch, a broken caller's wish is simply discarded and nobody
> will notice.  Without your patch, at least you will know that the caller
> passed an inconsistent pair of ac and av to this function by seeing a
> coalmine canary segfault.

OK. I will send another patch for verify_bundle() then :-) just to make sure no one goes down the same way.

-- 
Duy

Jeff King· May 21, 2009, 04:18 UTC · re: Junio C Hamano · lore

Re: [PATCH] setup_revisions(): do not access outside argv

On Wed, May 20, 2009 at 06:58:41PM -0700, Junio C Hamano wrote:

Show 14 quoted lines

> If a function takes (int ac, char **av), then people should be able to
> depend on the usual convention of
> 
>  (1) for any i < ac, av[i] is not NULL; and
>  (2) av[ac] is NULL.
> 
> With your patch, a broken caller's wish is simply discarded and nobody
> will notice.  Without your patch, at least you will know that the caller
> passed an inconsistent pair of ac and av to this function by seeing a
> coalmine canary segfault.
> 
> I would not mind a patch that adds an assertion that protects this
> function from broken callers, so that we can find them, but your patch
> makes me feel very uneasy.

I agree.

Having just fixed a segfault in the GIT_TRACE code caused by a non-terminated argv generated by the alias code, I think I would prefer that we just consistently do the NULL-termination. You are otherwise creating a maintenance pitfall when somebody later passes the value to unsuspecting code.

-Peff

Thomas Jarosch· May 21, 2009, 18:02 UTC · re: Jeff King · lore

Re: [PATCH] setup_revisions(): do not access outside argv

Jeff King wrote:

Show 5 quoted lines

> Having just fixed a segfault in the GIT_TRACE code caused by a
> non-terminated argv generated by the alias code, I think I would prefer
> that we just consistently do the NULL-termination. You are otherwise
> creating a maintenance pitfall when somebody later passes the value to
> unsuspecting code.

Speaking of that, there is also one piece of code in diff.c that doesn't do NULL-termination after a readlink() call (which never NULL-terminates). The current use is 100% fine, though the same maintenance argument might apply here, too. Wondering why the buffer is allocated as PATH_MAX +1. Hmm.

Thomas

Jeff King· May 22, 2009, 07:56 UTC · re: Thomas Jarosch · lore

Re: [PATCH] setup_revisions(): do not access outside argv

On Thu, May 21, 2009 at 08:02:08PM +0200, Thomas Jarosch wrote:

Show 5 quoted lines

> Speaking of that, there is also one piece of code in diff.c that doesn't do
> NULL-termination after a readlink() call (which never NULL-terminates).
> The current use is 100% fine, though the same maintenance
> argument might apply here, too. Wondering why the buffer
> is allocated as PATH_MAX +1. Hmm.

Yeah, it is fine because it just passes the result to prep_temp_blob, which respects the length. I don't know if it is worth making it more safe (arguably it should just be using strbuf_readlink anyway, but that does introduce an extra malloc).

I grepped and every other call to readlink is already doing this (and most just use strbuf_readlink anyway).

-- >8 --
Subject: NUL-terminate readlink results

readlink does not terminate its result, but instead returns the length of the path. This is not an actual bugfix, as the value is currently only used with its length. However, terminating the string helps make it safer for future uses.

Signed-off-by: Jeff King <peff@peff.net>
---
This does feel a bit like code churn, but I'm not sure it is any
different than the NULL-terminate all argv proposal.

Show changes to diff.c +1 −0

diff --git a/diff.c b/diff.c
index f06876b..b398360 100644
--- a/diff.c
+++ b/diff.c
@@ -2021,6 +2021,7 @@ static struct diff_tempfile *prepare_temp_file(const char *name,
 				die("readlink(%s)", name);
 			if (ret == sizeof(buf))
 				die("symlink too long: %s", name);
+			buf[ret] = '\0';
 			prep_temp_blob(name, temp, buf, ret,
 				       (one->sha1_valid ?
 					one->sha1 : null_sha1),

Jeff King· May 22, 2009, 08:02 UTC · re: Jeff King · lore

Re: [PATCH] setup_revisions(): do not access outside argv

On Fri, May 22, 2009 at 03:56:20AM -0400, Jeff King wrote:

> Yeah, it is fine because it just passes the result to prep_temp_blob,
> which respects the length. I don't know if it is worth making it more
> safe (arguably it should just be using strbuf_readlink anyway, but that
> does introduce an extra malloc).

And here is the strbuf_readlink version, which actually does make the source shorter and easier to read.

-- >8 --
Subject: [PATCH] convert bare readlink to strbuf_readlink

This particular readlink call never NUL-terminated its result, making it a potential source of bugs (though there is no bug now, as it currently always respects the length field). Let's just switch it to strbuf_readlink which is shorter and less error-prone.

Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

Show changes to diff.c +3 −7

diff --git a/diff.c b/diff.c
index f06876b..ffbe5c4 100644
--- a/diff.c
+++ b/diff.c
@@ -2014,14 +2014,10 @@ static struct diff_tempfile *prepare_temp_file(const char *name,
 			die("stat(%s): %s", name, strerror(errno));
 		}
 		if (S_ISLNK(st.st_mode)) {
-			int ret;
-			char buf[PATH_MAX + 1]; /* ought to be SYMLINK_MAX */
-			ret = readlink(name, buf, sizeof(buf));
-			if (ret < 0)
+			struct strbuf sb = STRBUF_INIT;
+			if (strbuf_readlink(&sb, name, st.st_size) < 0)
 				die("readlink(%s)", name);
-			if (ret == sizeof(buf))
-				die("symlink too long: %s", name);
-			prep_temp_blob(name, temp, buf, ret,
+			prep_temp_blob(name, temp, sb.buf, sb.len,
 				       (one->sha1_valid ?
 					one->sha1 : null_sha1),
 				       (one->sha1_valid ?

-- 
1.6.3.1.179.gec578.dirty

Thomas Jarosch· May 22, 2009, 14:23 UTC · re: Jeff King · lore

Re: [PATCH] setup_revisions(): do not access outside argv

On Friday, 22. May 2009 10:02:58 Jeff King wrote:

Show 8 quoted lines

> On Fri, May 22, 2009 at 03:56:20AM -0400, Jeff King wrote:
> > Yeah, it is fine because it just passes the result to prep_temp_blob,
> > which respects the length. I don't know if it is worth making it more
> > safe (arguably it should just be using strbuf_readlink anyway, but that
> > does introduce an extra malloc).
>
> And here is the strbuf_readlink version, which actually does make the
> source shorter and easier to read.

Good work! Patch looks fine to me. Guess you can't even benchmark the "extra" malloc ;-)

Have a nice weekend, Thomas

Brandon Casey· May 22, 2009, 15:33 UTC · re: Jeff King · lore

Re: [PATCH] setup_revisions(): do not access outside argv

Jeff King wrote:

Show 46 quoted lines

> On Fri, May 22, 2009 at 03:56:20AM -0400, Jeff King wrote:
> 
>> Yeah, it is fine because it just passes the result to prep_temp_blob,
>> which respects the length. I don't know if it is worth making it more
>> safe (arguably it should just be using strbuf_readlink anyway, but that
>> does introduce an extra malloc).
> 
> And here is the strbuf_readlink version, which actually does make the
> source shorter and easier to read.
> 
> -- >8 --
> Subject: [PATCH] convert bare readlink to strbuf_readlink
> 
> This particular readlink call never NUL-terminated its
> result, making it a potential source of bugs (though there
> is no bug now, as it currently always respects the length
> field). Let's just switch it to strbuf_readlink which is
> shorter and less error-prone.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  diff.c |   10 +++-------
>  1 files changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/diff.c b/diff.c
> index f06876b..ffbe5c4 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -2014,14 +2014,10 @@ static struct diff_tempfile *prepare_temp_file(const char *name,
>  			die("stat(%s): %s", name, strerror(errno));
>  		}
>  		if (S_ISLNK(st.st_mode)) {
> -			int ret;
> -			char buf[PATH_MAX + 1]; /* ought to be SYMLINK_MAX */
> -			ret = readlink(name, buf, sizeof(buf));
> -			if (ret < 0)
> +			struct strbuf sb = STRBUF_INIT;
> +			if (strbuf_readlink(&sb, name, st.st_size) < 0)
>  				die("readlink(%s)", name);
> -			if (ret == sizeof(buf))
> -				die("symlink too long: %s", name);
> -			prep_temp_blob(name, temp, buf, ret,
> +			prep_temp_blob(name, temp, sb.buf, sb.len,
>  				       (one->sha1_valid ?
>  					one->sha1 : null_sha1),
>  				       (one->sha1_valid ?

Don't you need to strbuf_release() ?

-brandon

Jeff King· May 22, 2009, 15:34 UTC · re: Brandon Casey · lore

Re: [PATCH] setup_revisions(): do not access outside argv

On Fri, May 22, 2009 at 10:33:22AM -0500, Brandon Casey wrote:

Show 17 quoted lines

> >  		if (S_ISLNK(st.st_mode)) {
> > -			int ret;
> > -			char buf[PATH_MAX + 1]; /* ought to be SYMLINK_MAX */
> > -			ret = readlink(name, buf, sizeof(buf));
> > -			if (ret < 0)
> > +			struct strbuf sb = STRBUF_INIT;
> > +			if (strbuf_readlink(&sb, name, st.st_size) < 0)
> >  				die("readlink(%s)", name);
> > -			if (ret == sizeof(buf))
> > -				die("symlink too long: %s", name);
> > -			prep_temp_blob(name, temp, buf, ret,
> > +			prep_temp_blob(name, temp, sb.buf, sb.len,
> >  				       (one->sha1_valid ?
> >  					one->sha1 : null_sha1),
> >  				       (one->sha1_valid ?
> 
> Don't you need to strbuf_release() ?

Urgh, yes, of course. Thanks for noticing.

-Peff

Jeff King· May 25, 2009, 10:46 UTC · re: Jeff King · lore

[PATCH] convert bare readlink to strbuf_readlink

Signed-off-by: Jeff King <peff@peff.net>
---
This is a re-post with the missing strbuf_release added.

I am not overly married to this patch, so if you think it is code churn, please just say so and drop it. I am just clearing out my "clean up and submit" queue. :)

 diff.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

Show changes to diff.c +4 −7

diff --git a/diff.c b/diff.c
index f06876b..dcfbcb0 100644
--- a/diff.c
+++ b/diff.c
@@ -2014,18 +2014,15 @@ static struct diff_tempfile *prepare_temp_file(const char *name,
 			die("stat(%s): %s", name, strerror(errno));
 		}
 		if (S_ISLNK(st.st_mode)) {
-			int ret;
-			char buf[PATH_MAX + 1]; /* ought to be SYMLINK_MAX */
-			ret = readlink(name, buf, sizeof(buf));
-			if (ret < 0)
+			struct strbuf sb = STRBUF_INIT;
+			if (strbuf_readlink(&sb, name, st.st_size) < 0)
 				die("readlink(%s)", name);
-			if (ret == sizeof(buf))
-				die("symlink too long: %s", name);
-			prep_temp_blob(name, temp, buf, ret,
+			prep_temp_blob(name, temp, sb.buf, sb.len,
 				       (one->sha1_valid ?
 					one->sha1 : null_sha1),
 				       (one->sha1_valid ?
 					one->mode : S_IFLNK));
+			strbuf_release(&sb);
 		}
 		else {
 			/* we can borrow from the file in the work tree */

-- 
1.6.3.1.250.g01b8b.dirty

Junio C Hamano· May 25, 2009, 22:23 UTC · re: Jeff King · lore

Re: [PATCH] convert bare readlink to strbuf_readlink

Jeff King <peff@peff.net> writes:

> This is a re-post with the missing strbuf_release added.

Thanks.

Jeff King· Jun 2, 2009, 13:57 UTC · lore

Re: [PATCH] setup_revisions(): do not access outside argv

On Tue, Jun 02, 2009 at 07:56:05PM +0900, Nanako Shiraishi wrote:

> > And here is the strbuf_readlink version, which actually does make the
> > source shorter and easier to read.
> 
> Junio, may I ask what happened to this patch?

I think it's now 3cd7388.

-Peff