Re: [PATCH v8 1/5] setup: add gentle version of read_gitfile

[Jeff King <peff@peff.net>]

On Mon, Jun 15, 2015 at 09:39:51PM +0200, Erik Elfström wrote:

> +cleanup_return:
>  	free(buf);
> +
> +	if (return_error_code)
> +		*return_error_code = error_code;
> +
> +	if (error_code) {
> +		if (return_error_code)
> +			return NULL;
> +
> +		switch (error_code) {
> +		case READ_GITFILE_ERR_STAT_FAILED:
> +		case READ_GITFILE_ERR_NOT_A_FILE:
> +			return NULL;
> +		case READ_GITFILE_ERR_OPEN_FAILED:
> +			die_errno("Error opening '%s'", path);
> +		case READ_GITFILE_ERR_READ_FAILED:
> +			die("Error reading %s", path);
> +		case READ_GITFILE_ERR_INVALID_FORMAT:
> +			die("Invalid gitfile format: %s", path);
> +		case READ_GITFILE_ERR_NO_PATH:
> +			die("No path in gitfile: %s", path);
> +		case READ_GITFILE_ERR_NOT_A_REPO:
> +			die("Not a git repository: %s", dir);
> +		default:
> +			assert(0);
> +		}

I happened to be playing with clang's static analyzer today, and it
noticed that there is a subtle use-after-free here. Here's a patch (on
top of ee/clean-remove-dirs, which is in 'next').

In practice I suspect it prints the right thing on most platforms, just
because nobody else has a chance to clobber the heap. But doing:

  echo "gitdir: /some/not-gitdir/path" >.git
  valgrind git status

does detect the problem.

-- >8 --
Subject: [PATCH] read_gitfile_gently: fix use-after-free

The "dir" variable is a pointer into the "buf" array. When
we hit the cleanup_return path, the first thing we do is
free(buf); but one of the error messages prints "dir", which
will access the memory after the free.

We can fix this by reorganizing the error path a little. We
act on the fatal, error-printing conditions first, as they
want to access memory and do not care about freeing. Then we
free any memory, and finally return.

Signed-off-by: Jeff King <peff@peff.net>
---
We can also spell the "else if" below as:

  if (error_code && !return_error_code)

but IMHO it reads better as I have it here: we report the error code if
the user asked for it, and otherwise follow the print-and-die path. We
could even spell it as just "else" and bump the "0" case down into the
switch statement.

 setup.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/setup.c b/setup.c
index 7b30f32..5eaca48 100644
--- a/setup.c
+++ b/setup.c
@@ -517,19 +517,14 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
 	path = real_path(dir);
 
 cleanup_return:
-	free(buf);
-
 	if (return_error_code)
 		*return_error_code = error_code;
-
-	if (error_code) {
-		if (return_error_code)
-			return NULL;
-
+	else if (error_code) {
 		switch (error_code) {
 		case READ_GITFILE_ERR_STAT_FAILED:
 		case READ_GITFILE_ERR_NOT_A_FILE:
-			return NULL;
+			/* non-fatal; follow return path */
+			break;
 		case READ_GITFILE_ERR_OPEN_FAILED:
 			die_errno("Error opening '%s'", path);
 		case READ_GITFILE_ERR_TOO_LARGE:
@@ -547,7 +542,8 @@ cleanup_return:
 		}
 	}
 
-	return path;
+	free(buf);
+	return error_code ? NULL : path;
 }
 
 static const char *setup_explicit_git_dir(const char *gitdirenv,
-- 
2.5.0.rc0.336.g8460790