Re: [PATCH v8 1/5] setup: add gentle version of read_gitfile
- From
Jeff King <peff@peff.net>
- Date
- Jun 26, 2015, 09:03 UTC
- Message-ID
- <20150626090331.GA4196@peff.net>
- In-Reply-To
- <1434397195-1823-2-git-send-email-erik.elfstrom@gmail.com>
On Mon, Jun 15, 2015 at 09:39:51PM +0200, Erik Elfström wrote:
Show 27 quoted lines
> +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