Re: [PATCH] config.c: handle error case for fstat() calls
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 19, 2016, 18:14 UTC
- Message-ID
- <xmqqtw9zpz0c.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20161219092155.20359-1-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > Will this fix the problem I'm replying to? I don't know. I found this > while checking the code and it should be fixed regardless.
Yeah, from a cursory read, it is a step in the right direction to check the return value of fstat().
Shouldn't the error-return path in the second hunk rollback the lockfile to clean after itself? The existing "Oh, we cannot chmod to match the original" check that comes immediately after shares the same issue, so this is not a new problem, but making an existing one worse.
Show 33 quoted lines
> config.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/config.c b/config.c
> index 83fdecb..4973256 100644
> --- a/config.c
> +++ b/config.c
> @@ -2194,7 +2194,12 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
> goto out_free;
> }
>
> - fstat(in_fd, &st);
> + if (fstat(in_fd, &st) == -1) {
> + error_errno(_("fstat on %s failed"), config_filename);
> + ret = CONFIG_INVALID_FILE;
> + goto out_free;
> + }
> +
> contents_sz = xsize_t(st.st_size);
> contents = xmmap_gently(NULL, contents_sz, PROT_READ,
> MAP_PRIVATE, in_fd, 0);
> @@ -2414,7 +2419,10 @@ int git_config_rename_section_in_file(const char *config_filename,
> goto unlock_and_out;
> }
>
> - fstat(fileno(config_file), &st);
> + if (fstat(fileno(config_file), &st) == -1) {
> + ret = error_errno(_("fstat on %s failed"), config_filename);
> + goto out;
> + }
>
> if (chmod(get_lock_file_path(lock), st.st_mode & 07777) < 0) {
> ret = error_errno("chmod on %s failed",