[PATCH] diff: avoid stack-buffer-read-overrun for very long name

[PATCH] diff: avoid stack-buffer-read-overrun for very long name

[Jim Meyering]

Due to the use of strncpy without explicit NUL termination,
we could end up passing names n1 or n2 that are not NUL-terminated
to queue_diff, which requires NUL-terminated strings.
Ensure that each is NUL terminated.

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
After finding strncpy problems in other projects, I audited
git for the same and found only these two.

 diff-no-index.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/diff-no-index.c b/diff-no-index.c
index 3a36144..5cd3ff5 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -109,6 +109,7 @@ static int queue_diff(struct diff_options *o,
 				n1 = buffer1;
 				strncpy(buffer1 + len1, p1.items[i1++].string,
 						PATH_MAX - len1);
+				buffer1[PATH_MAX-1] = 0;
 			}

 			if (comp < 0)
@@ -117,6 +118,7 @@ static int queue_diff(struct diff_options *o,
 				n2 = buffer2;
 				strncpy(buffer2 + len2, p2.items[i2++].string,
 						PATH_MAX - len2);
+				buffer2[PATH_MAX-1] = 0;
 			}

 			ret = queue_diff(o, n1, n2);
--
1.7.10.169.g146fe

Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name

[Marcus Karlsson]

On Mon, Apr 16, 2012 at 05:20:02PM +0200, Jim Meyering wrote:
> 
> Due to the use of strncpy without explicit NUL termination,
> we could end up passing names n1 or n2 that are not NUL-terminated
> to queue_diff, which requires NUL-terminated strings.
> Ensure that each is NUL terminated.
> 
> Signed-off-by: Jim Meyering <meyering@redhat.com>
> ---
> After finding strncpy problems in other projects, I audited
> git for the same and found only these two.
> 
>  diff-no-index.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/diff-no-index.c b/diff-no-index.c
> index 3a36144..5cd3ff5 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -109,6 +109,7 @@ static int queue_diff(struct diff_options *o,
>  				n1 = buffer1;
>  				strncpy(buffer1 + len1, p1.items[i1++].string,
>  						PATH_MAX - len1);
> +				buffer1[PATH_MAX-1] = 0;
>  			}
> 
>  			if (comp < 0)
> @@ -117,6 +118,7 @@ static int queue_diff(struct diff_options *o,
>  				n2 = buffer2;
>  				strncpy(buffer2 + len2, p2.items[i2++].string,
>  						PATH_MAX - len2);
> +				buffer2[PATH_MAX-1] = 0;
>  			}
> 
>  			ret = queue_diff(o, n1, n2);
> --
> 1.7.10.169.g146fe

Are there any guarantees that len1 and len2 does not exceed PATH_MAX?
Because if there aren't any then that function looks like it could need
even more improvements.

	Marcus

Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name

[Jim Meyering]

Marcus Karlsson wrote:
> On Mon, Apr 16, 2012 at 05:20:02PM +0200, Jim Meyering wrote:
>>
>> Due to the use of strncpy without explicit NUL termination,
>> we could end up passing names n1 or n2 that are not NUL-terminated
>> to queue_diff, which requires NUL-terminated strings.
>> Ensure that each is NUL terminated.
>>
>> Signed-off-by: Jim Meyering <meyering@redhat.com>
>> ---
>> After finding strncpy problems in other projects, I audited
>> git for the same and found only these two.
>>
>>  diff-no-index.c |    2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/diff-no-index.c b/diff-no-index.c
>> index 3a36144..5cd3ff5 100644
>> --- a/diff-no-index.c
>> +++ b/diff-no-index.c
>> @@ -109,6 +109,7 @@ static int queue_diff(struct diff_options *o,
>>  				n1 = buffer1;
>>  				strncpy(buffer1 + len1, p1.items[i1++].string,
>>  						PATH_MAX - len1);
>> +				buffer1[PATH_MAX-1] = 0;
>>  			}
>>
>>  			if (comp < 0)
>> @@ -117,6 +118,7 @@ static int queue_diff(struct diff_options *o,
>>  				n2 = buffer2;
>>  				strncpy(buffer2 + len2, p2.items[i2++].string,
>>  						PATH_MAX - len2);
>> +				buffer2[PATH_MAX-1] = 0;
>>  			}
>>
>>  			ret = queue_diff(o, n1, n2);
>> --
>> 1.7.10.169.g146fe
>
> Are there any guarantees that len1 and len2 does not exceed PATH_MAX?
> Because if there aren't any then that function looks like it could need
> even more improvements.

Hi Marcus,

You're right to ask.
I've just confirmed that there is such a guarantee.  The question
is whether either of queue_diff's name1 or name2 parameters may have
strlen larger than PATH_MAX, in which case, this code would misbehave,
passing a negative length to strncpy:

				strncpy(buffer1 + len1, p1.items[i1++].string,
						PATH_MAX - len1);
				buffer1[PATH_MAX-1] = 0;

queue_diff is called from only two places:

  - from itself, recursively
  - from diff_no_index

The latter looks fine, since it's called with already-vetted names:

	if (queue_diff(&revs->diffopt, revs->diffopt.pathspec.raw[0],
		       revs->diffopt.pathspec.raw[1]))

The recursive call is reachable only when both name1 and name2 are lstat'able.
If they're not (assuming they're non-trivial), this get_mode call fails:

    static int queue_diff(struct diff_options *o,
                    const char *name1, const char *name2)
    {
            int mode1 = 0, mode2 = 0;

            if (get_mode(name1, &mode1) || get_mode(name2, &mode2))
                    return -1;

Thus, as long as a file with name longer than PATH_MAX is not
lstat'able (what about hurd?), we're ok.

However, a further improvement is possible if you care what happens
when a very long newly-formed name is truncated by that use of strncpy.
When that happens, in a pathological case in which the truncated
name exists as well as the original, queue_diff could print totally
bogus results.

I.e., if dir/.../.../some-name is 5 bytes too long,
and the truncated "n1" formed in queue_diff, "dir/.../.../some"
refers to a file that actually exists, queue_diff will mistakenly
use the truncated file name.

Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name

[Junio C Hamano]

Jim Meyering <jim@meyering.net> writes:

> Marcus Karlsson wrote:
> ...
>> Are there any guarantees that len1 and len2 does not exceed PATH_MAX?
>> Because if there aren't any then that function looks like it could need
>> even more improvements.
>
> Hi Marcus,
>
> You're right to ask.
> I've just confirmed that there is such a guarantee.

In any case, I think this is an old part of the codebase that has not
been updated to take advantage of newer API, partly because not many
people cared, and partly because there wasn't any serious bug there,
that can use some facelifting.  Wouldn't it make more sense to use
strbuf here, perhaps like this (not even compile tested), on top of your
patch?

 diff-no-index.c |   40 +++++++++++++++++-----------------------
 1 file changed, 17 insertions(+), 23 deletions(-)

diff --git a/diff-no-index.c b/diff-no-index.c
index 5cd3ff5..b44473e 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -52,7 +52,7 @@ static int get_mode(const char *path, int *mode)
 }
 
 static int queue_diff(struct diff_options *o,
-		const char *name1, const char *name2)
+		      const char *name1, const char *name2)
 {
 	int mode1 = 0, mode2 = 0;
 
@@ -63,10 +63,11 @@ static int queue_diff(struct diff_options *o,
 		return error("file/directory conflict: %s, %s", name1, name2);
 
 	if (S_ISDIR(mode1) || S_ISDIR(mode2)) {
-		char buffer1[PATH_MAX], buffer2[PATH_MAX];
+		struct strbuf buffer1 = STRBUF_INIT;
+		struct strbuf buffer2 = STRBUF_INIT;
 		struct string_list p1 = STRING_LIST_INIT_DUP;
 		struct string_list p2 = STRING_LIST_INIT_DUP;
-		int len1 = 0, len2 = 0, i1, i2, ret = 0;
+		int i1, i2, ret = 0;
 
 		if (name1 && read_directory(name1, &p1))
 			return -1;
@@ -76,19 +77,15 @@ static int queue_diff(struct diff_options *o,
 		}
 
 		if (name1) {
-			len1 = strlen(name1);
-			if (len1 > 0 && name1[len1 - 1] == '/')
-				len1--;
-			memcpy(buffer1, name1, len1);
-			buffer1[len1++] = '/';
+			strbuf_addstr(&buffer1, name1);
+			if (buffer1.len && buffer1.buf[buffer1.len - 1] != '/')
+				strbuf_addch(&buffer1, '/');
 		}
 
 		if (name2) {
-			len2 = strlen(name2);
-			if (len2 > 0 && name2[len2 - 1] == '/')
-				len2--;
-			memcpy(buffer2, name2, len2);
-			buffer2[len2++] = '/';
+			strbuf_addstr(&buffer2, name2);
+			if (buffer2.len && buffer2.buf[buffer2.len - 1] != '/')
+				strbuf_addch(&buffer2, '/');
 		}
 
 		for (i1 = i2 = 0; !ret && (i1 < p1.nr || i2 < p2.nr); ) {
@@ -100,31 +97,28 @@ static int queue_diff(struct diff_options *o,
 			else if (i2 == p2.nr)
 				comp = -1;
 			else
-				comp = strcmp(p1.items[i1].string,
-					p2.items[i2].string);
+				comp = strcmp(p1.items[i1].string, p2.items[i2].string);
 
 			if (comp > 0)
 				n1 = NULL;
 			else {
-				n1 = buffer1;
-				strncpy(buffer1 + len1, p1.items[i1++].string,
-						PATH_MAX - len1);
-				buffer1[PATH_MAX-1] = 0;
+				strbuf_addstr(&buffer1, p1.items[i1++].string);
+				n1 = buffer1.buf;
 			}
 
 			if (comp < 0)
 				n2 = NULL;
 			else {
-				n2 = buffer2;
-				strncpy(buffer2 + len2, p2.items[i2++].string,
-						PATH_MAX - len2);
-				buffer2[PATH_MAX-1] = 0;
+				strbuf_addstr(&buffer2, p2.items[i2++].string);
+				n2 = buffer2.buf;
 			}
 
 			ret = queue_diff(o, n1, n2);
 		}
 		string_list_clear(&p1, 0);
 		string_list_clear(&p2, 0);
+		strbuf_reset(&buffer1);
+		strbuf_reset(&buffer2);
 
 		return ret;
 	} else {

Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name

[Jim Meyering]

Junio C Hamano wrote:
> Jim Meyering <jim@meyering.net> writes:
>
>> Marcus Karlsson wrote:
>> ...
>>> Are there any guarantees that len1 and len2 does not exceed PATH_MAX?
>>> Because if there aren't any then that function looks like it could need
>>> even more improvements.
>>
>> Hi Marcus,
>>
>> You're right to ask.
>> I've just confirmed that there is such a guarantee.
>
> In any case, I think this is an old part of the codebase that has not
> been updated to take advantage of newer API, partly because not many
> people cared, and partly because there wasn't any serious bug there,
> that can use some facelifting.  Wouldn't it make more sense to use
> strbuf here, perhaps like this (not even compile tested), on top of your
> patch?
>
>  diff-no-index.c |   40 +++++++++++++++++-----------------------
>  1 file changed, 17 insertions(+), 23 deletions(-)
>
> diff --git a/diff-no-index.c b/diff-no-index.c
> index 5cd3ff5..b44473e 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -52,7 +52,7 @@ static int get_mode(const char *path, int *mode)
>  }
>
>  static int queue_diff(struct diff_options *o,
> -		const char *name1, const char *name2)
> +		      const char *name1, const char *name2)
>  {
>  	int mode1 = 0, mode2 = 0;
>
> @@ -63,10 +63,11 @@ static int queue_diff(struct diff_options *o,
>  		return error("file/directory conflict: %s, %s", name1, name2);
>
>  	if (S_ISDIR(mode1) || S_ISDIR(mode2)) {
> -		char buffer1[PATH_MAX], buffer2[PATH_MAX];
> +		struct strbuf buffer1 = STRBUF_INIT;
> +		struct strbuf buffer2 = STRBUF_INIT;
>  		struct string_list p1 = STRING_LIST_INIT_DUP;
>  		struct string_list p2 = STRING_LIST_INIT_DUP;
> -		int len1 = 0, len2 = 0, i1, i2, ret = 0;
> +		int i1, i2, ret = 0;
>
>  		if (name1 && read_directory(name1, &p1))
>  			return -1;
> @@ -76,19 +77,15 @@ static int queue_diff(struct diff_options *o,
>  		}
>
>  		if (name1) {
> -			len1 = strlen(name1);
> -			if (len1 > 0 && name1[len1 - 1] == '/')
> -				len1--;
> -			memcpy(buffer1, name1, len1);
> -			buffer1[len1++] = '/';
> +			strbuf_addstr(&buffer1, name1);
> +			if (buffer1.len && buffer1.buf[buffer1.len - 1] != '/')
> +				strbuf_addch(&buffer1, '/');
>  		}
>
>  		if (name2) {
> -			len2 = strlen(name2);
> -			if (len2 > 0 && name2[len2 - 1] == '/')
> -				len2--;
> -			memcpy(buffer2, name2, len2);
> -			buffer2[len2++] = '/';
> +			strbuf_addstr(&buffer2, name2);
> +			if (buffer2.len && buffer2.buf[buffer2.len - 1] != '/')
> +				strbuf_addch(&buffer2, '/');

Hi Junio,

That looks much better.
I verified that it compiles and passes the tests on x86_64/Fedora 17.

What do you think about replacing those two append-if-needed two-liners:

    if (buffer2.len && buffer2.buf[buffer2.len - 1] != '/')
            strbuf_addch(&buffer2, '/');

by something that readably encapsulates the idiom:

    strbuf_append_if_absent (&buffer2, '/');

(though the name isn't particularly apt, because you might
take "absent" to mean "not anywhere in the string," so maybe
  strbuf_append_if_not_already_at_end (ugly) or
  strbuf_append_uniq
)

There are several other uses that would benefit from such a transformation:
To find the easy ones, I ran this:

  git grep -B1 "strbuf_addch.*'"|grep -A1 '!='

I've manually marked/separated the ones that don't apply.
Note how only 2 of the 6 candidates ensure that length is positive
before using ".len - 1":

------------------------------------
builtin/branch.c-	if (!buf.len || buf.buf[buf.len-1] != '\n')
builtin/branch.c:		strbuf_addch(&buf, '\n');
--
builtin/fmt-merge-msg.c-		if (out->buf[out->len - 1] != '\n')
builtin/fmt-merge-msg.c:			strbuf_addch(out, '\n');
--
builtin/log.c-		if (filename.buf[filename.len - 1] != '/')
builtin/log.c:			strbuf_addch(&filename, '/');
--
builtin/notes.c-	if (buf.buf[buf.len - 1] != '\n')
builtin/notes.c:		strbuf_addch(&buf, '\n'); /* Make sure msg ends with newline */
--
refs.c-		if (real_pattern.buf[real_pattern.len - 1] != '/')
refs.c:			strbuf_addch(&real_pattern, '/');
--
strbuf.h-	if (sb->len && sb->buf[sb->len - 1] != '\n')
strbuf.h:		strbuf_addch(sb, '\n');






--
NO wt-status.c-			if (*line != '\n' && *line != '\t')
NO wt-status.c:				strbuf_addch(&linebuf, ' ');
--
NO builtin/merge.c-	while ((commit = get_revision(&rev)) != NULL) {
NO builtin/merge.c:		strbuf_addch(&out, '\n');
--
NO builtin/shortlog.c-	if (col != log->wrap)
NO builtin/shortlog.c:		strbuf_addch(sb, '\n');
--
NO dir.c-	if (path->buf[original_len - 1] != '/')
NO dir.c:		strbuf_addch(path, '/');
--
NO path.c-	if (len && path[len-1] != '/')
NO path.c:		strbuf_addch(&buf, '/');
--
NO pretty.c-			if (p != commit->parents)
NO pretty.c:				strbuf_addch(sb, ' ');
--
NO pretty.c-			if (p != commit->parents)
NO pretty.c:				strbuf_addch(sb, ' ');
--
NO pretty.c-	if (pp->fmt != CMIT_FMT_ONELINE && !pp->subject) {
NO pretty.c:		strbuf_addch(sb, '\n');
--
NO pretty.c-	if (pp->fmt != CMIT_FMT_ONELINE)
NO pretty.c:		strbuf_addch(sb, '\n');

Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name

[Junio C Hamano]

Jim Meyering <jim@meyering.net> writes:

> What do you think about replacing those two append-if-needed two-liners:
>
>     if (buffer2.len && buffer2.buf[buffer2.len - 1] != '/')
>             strbuf_addch(&buffer2, '/');
>
> by something that readably encapsulates the idiom:
>
>     strbuf_append_if_absent (&buffer2, '/');
>
> (though the name isn't particularly apt, because you might
> take "absent" to mean "not anywhere in the string," so maybe
>   strbuf_append_if_not_already_at_end (ugly) or
>   strbuf_append_uniq
> )

I am not good at names, but strbuf_terminate_with(&buffer2, '/')
perhaps?

> There are several other uses that would benefit from such a transformation:
> To find the easy ones, I ran this:
>
>   git grep -B1 "strbuf_addch.*'"|grep -A1 '!='
>
> I've manually marked/separated the ones that don't apply.
>
> Note how only 2 of the 6 candidates ensure that length is positive
> before using ".len - 1":

Yikes, that is embarrasing ;-)

>
> ------------------------------------
> builtin/branch.c-	if (!buf.len || buf.buf[buf.len-1] != '\n')
> builtin/branch.c:		strbuf_addch(&buf, '\n');
> --
> builtin/fmt-merge-msg.c-		if (out->buf[out->len - 1] != '\n')
> builtin/fmt-merge-msg.c:			strbuf_addch(out, '\n');
> --
> builtin/log.c-		if (filename.buf[filename.len - 1] != '/')
> builtin/log.c:			strbuf_addch(&filename, '/');
> --
> builtin/notes.c-	if (buf.buf[buf.len - 1] != '\n')
> builtin/notes.c:		strbuf_addch(&buf, '\n'); /* Make sure msg ends with newline */
> --
> refs.c-		if (real_pattern.buf[real_pattern.len - 1] != '/')
> refs.c:			strbuf_addch(&real_pattern, '/');
> --
> strbuf.h-	if (sb->len && sb->buf[sb->len - 1] != '\n')
> strbuf.h:		strbuf_addch(sb, '\n');

Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name

[Bert Wesarg]

On Thu, Apr 26, 2012 at 18:13, Junio C Hamano <gitster@pobox.com> wrote:
> Jim Meyering <jim@meyering.net> writes:
>
>> What do you think about replacing those two append-if-needed two-liners:
>>
>>     if (buffer2.len && buffer2.buf[buffer2.len - 1] != '/')
>>             strbuf_addch(&buffer2, '/');
>>
>> by something that readably encapsulates the idiom:
>>
>>     strbuf_append_if_absent (&buffer2, '/');
>>
>> (though the name isn't particularly apt, because you might
>> take "absent" to mean "not anywhere in the string," so maybe
>>   strbuf_append_if_not_already_at_end (ugly) or
>>   strbuf_append_uniq
>> )
>
> I am not good at names, but strbuf_terminate_with(&buffer2, '/')
> perhaps?

strbuf_ensure_terminator(struct strbuf* buf, int term, int always)?

>
>> There are several other uses that would benefit from such a transformation:
>> To find the easy ones, I ran this:
>>
>>   git grep -B1 "strbuf_addch.*'"|grep -A1 '!='
>>
>> I've manually marked/separated the ones that don't apply.
>>
>> ------------------------------------
>> builtin/branch.c-     if (!buf.len || buf.buf[buf.len-1] != '\n')
>> builtin/branch.c:             strbuf_addch(&buf, '\n');
>> --
>> strbuf.h-     if (sb->len && sb->buf[sb->len - 1] != '\n')
>> strbuf.h:             strbuf_addch(sb, '\n');

Please note, that while they are checking the .len, they both behave
differently if .len == 0 or not.
The first always append a '\n', the latter only, if the string isn't empty.

Bert

Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name

[Jim Meyering]

Junio C Hamano wrote:
> Jim Meyering <jim@meyering.net> writes:
>
>> What do you think about replacing those two append-if-needed two-liners:
>>
>>     if (buffer2.len && buffer2.buf[buffer2.len - 1] != '/')
>>             strbuf_addch(&buffer2, '/');
>>
>> by something that readably encapsulates the idiom:
>>
>>     strbuf_append_if_absent (&buffer2, '/');
>>
>> (though the name isn't particularly apt, because you might
>> take "absent" to mean "not anywhere in the string," so maybe
>>   strbuf_append_if_not_already_at_end (ugly) or
>>   strbuf_append_uniq
>> )
>
> I am not good at names, but strbuf_terminate_with(&buffer2, '/')
> perhaps?

Maybe, but it still doesn't evoke the conditional nature
of don't-append-if-already-there the operation.  i.e., one
might wonder how it's different from "strbuf_append".

How about one of these?

  strbuf_ensure_suffix  // but might make you think suffix==more than 1 byte
  strbuf_ensure_last_byte    // maybe?
  strbuf_ensure_last_byte_is // rather long, but apt

>> There are several other uses that would benefit from such a transformation:
>> To find the easy ones, I ran this:
>>
>>   git grep -B1 "strbuf_addch.*'"|grep -A1 '!='
>>
>> I've manually marked/separated the ones that don't apply.
>>
>> Note how only 2 of the 6 candidates ensure that length is positive
>> before using ".len - 1":
>
> Yikes, that is embarrasing ;-)

Knowing you/git, each is because the buffer is known to be non-empty.

Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name

[Jim Meyering]

Bert Wesarg wrote:
> On Thu, Apr 26, 2012 at 18:13, Junio C Hamano <gitster@pobox.com> wrote:
>> Jim Meyering <jim@meyering.net> writes:
>>
>>> What do you think about replacing those two append-if-needed two-liners:
>>>
>>>     if (buffer2.len && buffer2.buf[buffer2.len - 1] != '/')
>>>             strbuf_addch(&buffer2, '/');
>>>
>>> by something that readably encapsulates the idiom:
>>>
>>>     strbuf_append_if_absent (&buffer2, '/');
>>>
>>> (though the name isn't particularly apt, because you might
>>> take "absent" to mean "not anywhere in the string," so maybe
>>>   strbuf_append_if_not_already_at_end (ugly) or
>>>   strbuf_append_uniq
>>> )
>>
>> I am not good at names, but strbuf_terminate_with(&buffer2, '/')
>> perhaps?
>
> strbuf_ensure_terminator(struct strbuf* buf, int term, int always)?

Nice!  So far, that's the name I prefer.
But why the third parameter?

Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name

[Bert Wesarg]

On Thu, Apr 26, 2012 at 18:26, Jim Meyering <jim@meyering.net> wrote:
> Bert Wesarg wrote:
>> On Thu, Apr 26, 2012 at 18:13, Junio C Hamano <gitster@pobox.com> wrote:
>>> Jim Meyering <jim@meyering.net> writes:
>> strbuf_ensure_terminator(struct strbuf* buf, int term, int always)?
>
> Nice!  So far, that's the name I prefer.
> But why the third parameter?

See the second part of my reply:

>>> ------------------------------------
>>> builtin/branch.c-     if (!buf.len || buf.buf[buf.len-1] != '\n')
>>> builtin/branch.c:             strbuf_addch(&buf, '\n');
>>> --
>>> strbuf.h-     if (sb->len && sb->buf[sb->len - 1] != '\n')
>>> strbuf.h:             strbuf_addch(sb, '\n');
>
> Please note, that while they are checking the .len, they both behave
> differently if .len == 0 or not.
> The first always append a '\n', the latter only, if the string isn't empty.

Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name

[Jim Meyering]

Bert Wesarg wrote:
> On Thu, Apr 26, 2012 at 18:26, Jim Meyering <jim@meyering.net> wrote:
>> Bert Wesarg wrote:
>>> On Thu, Apr 26, 2012 at 18:13, Junio C Hamano <gitster@pobox.com> wrote:
>>>> Jim Meyering <jim@meyering.net> writes:
>>> strbuf_ensure_terminator(struct strbuf* buf, int term, int always)?
>>
>> Nice!  So far, that's the name I prefer.
>> But why the third parameter?
>
> See the second part of my reply:

Oh.  I missed that.

>>>> ------------------------------------
>>>> builtin/branch.c-     if (!buf.len || buf.buf[buf.len-1] != '\n')
>>>> builtin/branch.c:             strbuf_addch(&buf, '\n');
>>>> --
>>>> strbuf.h-     if (sb->len && sb->buf[sb->len - 1] != '\n')
>>>> strbuf.h:             strbuf_addch(sb, '\n');
>>
>> Please note, that while they are checking the .len, they both behave
>> differently if .len == 0 or not.
>> The first always append a '\n', the latter only, if the string isn't empty.

Glad you noticed the difference.
However, is one exception worth complicating the interface?

Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name

[Andreas Ericsson]

On 04/26/2012 06:13 PM, Junio C Hamano wrote:
> Jim Meyering<jim@meyering.net>  writes:
> 
>> What do you think about replacing those two append-if-needed two-liners:
>>
>>      if (buffer2.len&&  buffer2.buf[buffer2.len - 1] != '/')
>>              strbuf_addch(&buffer2, '/');
>>
>> by something that readably encapsulates the idiom:
>>
>>      strbuf_append_if_absent (&buffer2, '/');
>>
>> (though the name isn't particularly apt, because you might
>> take "absent" to mean "not anywhere in the string," so maybe
>>    strbuf_append_if_not_already_at_end (ugly) or
>>    strbuf_append_uniq
>> )
> 
> I am not good at names, but strbuf_terminate_with(&buffer2, '/')
> perhaps?
> 

"terminate" sounds pretty final though. How about strbuf_ensure_suffixch()?
It embeds the 'ch', marking it as a char argument and provides natural names
for
  strbuf_ensure_suffix(buf, char *str);
  strbuf_ensure_prefix(buf, char *str);
  strbuf_ensure_prefixch(buf, char c);
if those are ever needed.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.

Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name

[Junio C Hamano]

Andreas Ericsson <ae@op5.se> writes:

>> I am not good at names, but strbuf_terminate_with(&buffer2, '/')
>> perhaps?
>> 
> "terminate" sounds pretty final though.

Yeah, but that function is adding a record terminator to the buffer, so...

> How about strbuf_ensure_suffixch()?
> It embeds the 'ch', marking it as a char argument...

Perhaps.