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

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

[Nguyễn Thái Ngọc Duy]

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;
 	}
-- 
test

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

[Johannes Sixt]

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.
If this is a bug, why did noboy notice this earlier? IOW, your commit
message is really lacking justification.

-- Hannes

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

[Nguyen Thai Ngoc Duy]

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.
-- 
Duy

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

[Junio C Hamano]

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.

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.

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

[Miles Bader]

Junio C Hamano <gitster@pobox.com> writes:
> 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

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

[Nguyen Thai Ngoc Duy]

On Thu, May 21, 2009 at 11:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Jeff King]

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

> 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

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

[Thomas Jarosch]

Jeff King wrote:
> 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

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

[Jeff King]

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

> 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.

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),

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

[Jeff King]

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 ?
-- 
1.6.3.1.179.gec578.dirty

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

[Thomas Jarosch]

On Friday, 22. May 2009 10:02:58 Jeff King wrote:
> 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

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

[Brandon Casey]

Jeff King wrote:
> 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

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

[Jeff King]

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

> >  		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

[PATCH] convert bare readlink to strbuf_readlink

[Jeff King]

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>
---
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(-)

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

Re: [PATCH] convert bare readlink to strbuf_readlink

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Thanks.

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

[Jeff King]

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