From a93bedada88dc15b0143708e1cb87c8fe9b9c705 Mon Sep 17 00:00:00 2001 From: Erik Elfström Date: Tue, 9 Jun 2015 20:24:35 +0200 Subject: setup: add gentle version of read_gitfile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Helped-by: Jeff King Signed-off-by: Erik Elfström Signed-off-by: Junio C Hamano --- setup.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 65 insertions(+), 19 deletions(-) (limited to 'setup.c') diff --git a/setup.c b/setup.c index 863ddfd938..4748b6338d 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: + 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); + } + } + return path; } -- cgit v1.3 From 921bdd96afc17ca055af261066eabdf026cb2195 Mon Sep 17 00:00:00 2001 From: Erik Elfström Date: Mon, 15 Jun 2015 21:39:52 +0200 Subject: setup: sanity check file size in read_gitfile_gently MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit read_gitfile_gently will allocate a buffer to fit the entire file that should be read. Add a sanity check of the file size before opening to avoid allocating a potentially huge amount of memory if we come across a large file that someone happened to name ".git". The limit is set to a sufficiently unreasonable size that should never be exceeded by a genuine .git file. Signed-off-by: Erik Elfström Signed-off-by: Junio C Hamano --- cache.h | 1 + setup.c | 7 +++++++ 2 files changed, 8 insertions(+) (limited to 'setup.c') diff --git a/cache.h b/cache.h index 25578cbf3c..858d9b3688 100644 --- a/cache.h +++ b/cache.h @@ -454,6 +454,7 @@ extern const char *get_git_work_tree(void); #define READ_GITFILE_ERR_INVALID_FORMAT 5 #define READ_GITFILE_ERR_NO_PATH 6 #define READ_GITFILE_ERR_NOT_A_REPO 7 +#define READ_GITFILE_ERR_TOO_LARGE 8 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); diff --git a/setup.c b/setup.c index 4748b6338d..a03ca94234 100644 --- a/setup.c +++ b/setup.c @@ -414,6 +414,7 @@ static void update_linked_gitdir(const char *gitfile, const char *gitdir) */ const char *read_gitfile_gently(const char *path, int *return_error_code) { + const int max_file_size = 1 << 20; /* 1MB */ int error_code = 0; char *buf = NULL; char *dir = NULL; @@ -430,6 +431,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code) error_code = READ_GITFILE_ERR_NOT_A_FILE; goto cleanup_return; } + if (st.st_size > max_file_size) { + error_code = READ_GITFILE_ERR_TOO_LARGE; + goto cleanup_return; + } fd = open(path, O_RDONLY); if (fd < 0) { error_code = READ_GITFILE_ERR_OPEN_FAILED; @@ -489,6 +494,8 @@ cleanup_return: return NULL; case READ_GITFILE_ERR_OPEN_FAILED: die_errno("Error opening '%s'", path); + case READ_GITFILE_ERR_TOO_LARGE: + die("Too large to be a .git file: '%s'", path); case READ_GITFILE_ERR_READ_FAILED: die("Error reading %s", path); case READ_GITFILE_ERR_INVALID_FORMAT: -- cgit v1.3 From 38ae8784074852c8e7b651f4f6e44e07466da7e1 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 26 Jun 2015 05:03:31 -0400 Subject: 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 Signed-off-by: Junio C Hamano --- setup.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) (limited to 'setup.c') diff --git a/setup.c b/setup.c index a03ca94234..97bb5e3b93 100644 --- a/setup.c +++ b/setup.c @@ -479,19 +479,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: @@ -509,7 +504,8 @@ cleanup_return: } } - return path; + free(buf); + return error_code ? NULL : path; } static const char *setup_explicit_git_dir(const char *gitdirenv, -- cgit v1.3