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 <[email protected]>
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
