From 4467aeac421efd04bc8c814541535c02853e9418 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 28 Mar 2017 09:00:48 +0200 Subject: [PATCH 1/5] config_file: handle errors other than OOM while parsing section headers The current code in `parse_section_header_ext` is only prepared to properly handle out-of-memory conditions for the `git_buf` structure. While very unlikely and probably caused by a programming error, it is also possible to run into error conditions other than out-of-memory previous to reaching the actual parsing loop. In these cases, we will run into undefined behavior as the `rpos` variable is only initialized after these triggerable errors, but we use it in the cleanup-routine. Fix the issue by unifying the function's cleanup code with an `end_error` section, which will not use the `rpos` variable. --- src/config_file.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/config_file.c b/src/config_file.c index 50c5a3d82..7df43c85f 100644 --- a/src/config_file.c +++ b/src/config_file.c @@ -1027,7 +1027,7 @@ static int parse_section_header_ext(struct reader *reader, const char *line, con first_quote = strchr(line, '"'); if (first_quote == NULL) { set_parse_error(reader, 0, "Missing quotation marks in section header"); - return -1; + goto end_error; } last_quote = strrchr(line, '"'); @@ -1035,7 +1035,7 @@ static int parse_section_header_ext(struct reader *reader, const char *line, con if (quoted_len == 0) { set_parse_error(reader, 0, "Missing closing quotation mark in section header"); - return -1; + goto end_error; } GITERR_CHECK_ALLOC_ADD(&alloc_len, base_name_len, quoted_len); @@ -1043,7 +1043,7 @@ static int parse_section_header_ext(struct reader *reader, const char *line, con if (git_buf_grow(&buf, alloc_len) < 0 || git_buf_printf(&buf, "%s.", base_name) < 0) - goto end_parse; + goto end_error; rpos = 0; @@ -1059,8 +1059,7 @@ static int parse_section_header_ext(struct reader *reader, const char *line, con switch (c) { case 0: set_parse_error(reader, 0, "Unexpected end-of-line in section header"); - git_buf_free(&buf); - return -1; + goto end_error; case '"': goto end_parse; @@ -1070,8 +1069,7 @@ static int parse_section_header_ext(struct reader *reader, const char *line, con if (c == 0) { set_parse_error(reader, rpos, "Unexpected end-of-line in section header"); - git_buf_free(&buf); - return -1; + goto end_error; } default: @@ -1083,10 +1081,8 @@ static int parse_section_header_ext(struct reader *reader, const char *line, con } while (line + rpos < last_quote); end_parse: - if (git_buf_oom(&buf)) { - git_buf_free(&buf); - return -1; - } + if (git_buf_oom(&buf)) + goto end_error; if (line[rpos] != '"' || line[rpos + 1] != ']') { set_parse_error(reader, rpos, "Unexpected text after closing quotes"); @@ -1096,6 +1092,11 @@ end_parse: *section_name = git_buf_detach(&buf); return 0; + +end_error: + git_buf_free(&buf); + + return -1; } static int parse_section_header(struct reader *reader, char **section_out) From cffd616a7205d6b025f3736eb6c0a1ae9c3cd85d Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 28 Mar 2017 09:08:41 +0200 Subject: [PATCH 2/5] path: handle error returned by `git_buf_joinpath` In the `_check_dir_contents` function, we first allocate memory for joining the directory and subdirectory together and afterwards use `git_buf_joinpath`. While this function in fact should not fail as memory is already allocated, err on the safe side and check for returned errors. --- src/path.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/path.c b/src/path.c index c3d3eb1ce..9b15c8cf9 100644 --- a/src/path.c +++ b/src/path.c @@ -700,7 +700,8 @@ static bool _check_dir_contents( return false; /* save excursion */ - git_buf_joinpath(dir, dir->ptr, sub); + if (git_buf_joinpath(dir, dir->ptr, sub) < 0) + return false; result = predicate(dir->ptr); From a76d75021c50315ab52d7a4e60419dc2e64c47e9 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 28 Mar 2017 09:12:34 +0200 Subject: [PATCH 3/5] path: short-circuit `git_path_apply_relative` on error Short-circuit the call to `git_path_resolve_relative` in case `git_buf_joinpath` returns an error. While this does not fix any immediate errors, the resulting code is easier to read and handles potential new error conditions raised by `git_buf_joinpath`. --- src/path.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/path.c b/src/path.c index 9b15c8cf9..b7205a646 100644 --- a/src/path.c +++ b/src/path.c @@ -826,8 +826,8 @@ int git_path_resolve_relative(git_buf *path, size_t ceiling) int git_path_apply_relative(git_buf *target, const char *relpath) { - git_buf_joinpath(target, git_buf_cstr(target), relpath); - return git_path_resolve_relative(target, 0); + return git_buf_joinpath(target, git_buf_cstr(target), relpath) || + git_path_resolve_relative(target, 0); } int git_path_cmp( From 756138e4755c93265e34704ee95874c0d81425e5 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 28 Mar 2017 09:15:53 +0200 Subject: [PATCH 4/5] blame_git: check return value of `git__calloc` We do not check the return value of `git__calloc`, which may return `NULL` in out-of-memory situations. Fix the error by using `GITERR_CHECK_ALLOC`. --- src/blame_git.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/blame_git.c b/src/blame_git.c index 6d2f1531f..13f5cb47c 100644 --- a/src/blame_git.c +++ b/src/blame_git.c @@ -517,11 +517,12 @@ static int pass_blame(git_blame *blame, git_blame__origin *origin, uint32_t opt) if (!num_parents) { git_oid_cpy(&blame->options.oldest_commit, git_commit_id(commit)); goto finish; - } - else if (num_parents < (int)ARRAY_SIZE(sg_buf)) + } else if (num_parents < (int)ARRAY_SIZE(sg_buf)) memset(sg_buf, 0, sizeof(sg_buf)); - else + else { sg_origin = git__calloc(num_parents, sizeof(*sg_origin)); + GITERR_CHECK_ALLOC(sg_origin); + } for (i=0; i Date: Tue, 28 Mar 2017 10:12:23 +0200 Subject: [PATCH 5/5] fileops: do not overwrite correct error message on mmap When executing `git_futils_mmap_ro_file`, we first try to guess whether the file is mmapable at all. Part of this check is whether the file is too large to be mmaped, which can be true on systems with 32 bit `size_t` types. The check is performed by first getting the file size wtih `git_futils_filesize` and then checking whether the returned size can be represented as `size_t`, returning an error if so. While this test also catches the case where the function returned an error (as `-1` is not representable by `size_t`), we will set the misleading error message "file too large to mmap". But in fact, a negative return value from `git_futils_filesize` will be caused by the inability to fstat the file. Fix the error message by handling negative return values separately and not overwriting the error message in that case. --- src/fileops.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/fileops.c b/src/fileops.c index ad2a988a9..f9552a5f8 100644 --- a/src/fileops.c +++ b/src/fileops.c @@ -304,7 +304,9 @@ int git_futils_mmap_ro_file(git_map *out, const char *path) if (fd < 0) return fd; - len = git_futils_filesize(fd); + if ((len = git_futils_filesize(fd)) < 0) + return -1; + if (!git__is_sizet(len)) { giterr_set(GITERR_OS, "file `%s` too large to mmap", path); return -1;