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

[Marcus Karlsson <mk@acc.umu.se>]

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