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

[Jeff King <peff@peff.net>]

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