Re: [PATCH v8 1/5] setup: add gentle version of read_gitfile
On Fri, Jun 26, 2015 at 11:03 AM, Jeff King p...@peff.net wrote: I happened to be playing with clang's static analyzer today, and it noticed that there is a subtle use-after-free here. Doh, sorry about that. Thanks for fixing my bug. /Erik -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 1/5] setup: add gentle version of read_gitfile
erik elfström erik.elfst...@gmail.com writes: On Fri, Jun 26, 2015 at 11:03 AM, Jeff King p...@peff.net wrote: I happened to be playing with clang's static analyzer today, and it noticed that there is a subtle use-after-free here. Doh, sorry about that. Thanks for fixing my bug. I missed that one while reviewing and queuing. Thanks, both. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 1/5] setup: add gentle version of read_gitfile
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 p...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v8 1/5] setup: add gentle version of read_gitfile
read_gitfile will die on most error cases. This makes it unsuitable for speculative calls. Extract the core logic and provide a gentle version that returns NULL on failure. The first usecase of the new gentle version will be to probe for submodules during git clean. Helped-by: Junio C Hamano gits...@pobox.com Helped-by: Jeff King p...@peff.net Signed-off-by: Erik Elfström erik.elfst...@gmail.com --- cache.h | 11 - setup.c | 84 ++--- 2 files changed, 75 insertions(+), 20 deletions(-) diff --git a/cache.h b/cache.h index 571c98f..25578cb 100644 --- a/cache.h +++ b/cache.h @@ -446,7 +446,16 @@ extern int get_common_dir(struct strbuf *sb, const char *gitdir); extern const char *get_git_namespace(void); extern const char *strip_namespace(const char *namespaced_ref); extern const char *get_git_work_tree(void); -extern const char *read_gitfile(const char *path); + +#define READ_GITFILE_ERR_STAT_FAILED 1 +#define READ_GITFILE_ERR_NOT_A_FILE 2 +#define READ_GITFILE_ERR_OPEN_FAILED 3 +#define READ_GITFILE_ERR_READ_FAILED 4 +#define READ_GITFILE_ERR_INVALID_FORMAT 5 +#define READ_GITFILE_ERR_NO_PATH 6 +#define READ_GITFILE_ERR_NOT_A_REPO 7 +extern const char *read_gitfile_gently(const char *path, int *return_error_code); +#define read_gitfile(path) read_gitfile_gently((path), NULL) extern const char *resolve_gitdir(const char *suspect); extern void set_git_work_tree(const char *tree); diff --git a/setup.c b/setup.c index 863ddfd..4748b63 100644 --- a/setup.c +++ b/setup.c @@ -406,35 +406,53 @@ static void update_linked_gitdir(const char *gitfile, const char *gitdir) /* * Try to read the location of the git directory from the .git file, * return path to git directory if found. + * + * On failure, if return_error_code is not NULL, return_error_code + * will be set to an error code and NULL will be returned. If + * return_error_code is NULL the function will die instead (for most + * cases). */ -const char *read_gitfile(const char *path) +const char *read_gitfile_gently(const char *path, int *return_error_code) { - char *buf; - char *dir; + int error_code = 0; + char *buf = NULL; + char *dir = NULL; const char *slash; struct stat st; int fd; ssize_t len; - if (stat(path, st)) - return NULL; - if (!S_ISREG(st.st_mode)) - return NULL; + if (stat(path, st)) { + error_code = READ_GITFILE_ERR_STAT_FAILED; + goto cleanup_return; + } + if (!S_ISREG(st.st_mode)) { + error_code = READ_GITFILE_ERR_NOT_A_FILE; + goto cleanup_return; + } fd = open(path, O_RDONLY); - if (fd 0) - die_errno(Error opening '%s', path); + if (fd 0) { + error_code = READ_GITFILE_ERR_OPEN_FAILED; + goto cleanup_return; + } buf = xmalloc(st.st_size + 1); len = read_in_full(fd, buf, st.st_size); close(fd); - if (len != st.st_size) - die(Error reading %s, path); + if (len != st.st_size) { + error_code = READ_GITFILE_ERR_READ_FAILED; + goto cleanup_return; + } buf[len] = '\0'; - if (!starts_with(buf, gitdir: )) - die(Invalid gitfile format: %s, path); + if (!starts_with(buf, gitdir: )) { + error_code = READ_GITFILE_ERR_INVALID_FORMAT; + goto cleanup_return; + } while (buf[len - 1] == '\n' || buf[len - 1] == '\r') len--; - if (len 9) - die(No path in gitfile: %s, path); + if (len 9) { + error_code = READ_GITFILE_ERR_NO_PATH; + goto cleanup_return; + } buf[len] = '\0'; dir = buf + 8; @@ -448,14 +466,42 @@ const char *read_gitfile(const char *path) free(buf); buf = dir; } - - if (!is_git_directory(dir)) - die(Not a git repository: %s, dir); - + if (!is_git_directory(dir)) { + error_code = READ_GITFILE_ERR_NOT_A_REPO; + goto cleanup_return; + } update_linked_gitdir(path, dir); 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; + + 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: