Re: [PATCH v4 2/5] setup: sanity check file size in read_gitfile_gently

[Junio C Hamano <gitster@pobox.com>]

Erik Elfström <erik.elfstrom@gmail.com> 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 <erik.elfstrom@gmail.com>
> ---
>
> 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.

>
>  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);