Re: [PATCH v4 2/5] setup: sanity check file size in read_gitfile_gently
flat view
From: Junio C Hamano <hidden>
Date: 2016-06-15 23:04:32
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH v4 2/5] setup: sanity check file size in read_gitfile_gently · Erik Elfström <hidden>
- 2016-06-15 · Re: [PATCH v4 2/5] setup: sanity check file size in read_gitfile_gently · Junio C Hamano <hidden>
Erik Elfström [off-list ref] writes:
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 <redacted> --- I'm not sure about this one but it felt like the safe thing to do. This patch can be dropped if it is not desired.
I do not think it is wrong per-se, but the changes in this patch shows why hardcoded values assigned to error_code without #define is not a good idea, as these values are now exposed to the callers of the new function. After we gain a new caller that does care about the exact error code (e.g. to react differently to the reason why we failed to read by giving different error messages) if we decide to revert this step, or if we decide to add a new error type, for whatever reason, this organization forces the caller to be updated.
I considered testing it using "mkdir foo && truncate -s 200G foo/.git && git clean -f -d" but that feels like a pretty evil test that is likely to cause lots of problems and not fail in any good way.
Amen to that.
quoted hunk
setup.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)diff --git a/setup.c b/setup.c index e1897cc..ed87334 100644 --- a/setup.c +++ b/setup.c@@ -364,22 +364,26 @@ const char *read_gitfile_gently(const char *path, int *return_error_code) error_code = 3; goto cleanup_return; } + if (st.st_size > PATH_MAX * 4) { + error_code = 4; + 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) { - error_code = 4; + error_code = 5; goto cleanup_return; } buf[len] = '\0'; if (!starts_with(buf, "gitdir: ")) { - error_code = 5; + error_code = 6; goto cleanup_return; } while (buf[len - 1] == '\n' || buf[len - 1] == '\r') len--; if (len < 9) { - error_code = 6; + error_code = 7; goto cleanup_return; } buf[len] = '\0';@@ -397,7 +401,7 @@ const char *read_gitfile_gently(const char *path, int *return_error_code) } if (!is_git_directory(dir)) { - error_code = 7; + error_code = 8; goto cleanup_return; } path = real_path(dir);@@ -419,12 +423,14 @@ cleanup_return: case 3: die_errno("Error opening '%s'", path); case 4: - die("Error reading %s", path); + die("Too large to be a .git file: '%s'", path); case 5: - die("Invalid gitfile format: %s", path); + die("Error reading %s", path); case 6: - die("No path in gitfile: %s", path); + die("Invalid gitfile format: %s", path); case 7: + die("No path in gitfile: %s", path); + case 8: die("Not a git repository: %s", dir); default: assert(0);