Re: [PATCH] config.c: handle error case for fstat() calls

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 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.

On Mon, Dec 19, 2016 at 10:14:27AM -0800, Junio C Hamano wrote:

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.

OK. How about two more patches on top (or bottom, does not matter)?
The second one should fix this. The first is sort of "good to do".

[PATCH 1/2] config.c: rename label unlock_and_out
[PATCH 2/2] config.c: handle lock file in error case in git_config_rename_...
--
Duy

There are two ways to unlock a file: commit, or revert. Rename it to
commit_and_out to avoid confusion.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 config.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 4973256..505e0d0 100644
--- a/config.c
+++ b/config.c

@@ -2416,7 +2416,7 @@ int git_config_rename_section_in_file(const char *config_filename,
 
 	if (!(config_file = fopen(config_filename, "rb"))) {
 		/* no config file means nothing to rename, no error */
-		goto unlock_and_out;
+		goto commit_and_out;
 	}
 
 	if (fstat(fileno(config_file), &st) == -1) {

@@ -2478,7 +2478,7 @@ int git_config_rename_section_in_file(const char *config_filename,
 		}
 	}
 	fclose(config_file);
-unlock_and_out:
+commit_and_out:
 	if (commit_lock_file(lock) < 0)
 		ret = error_errno("could not write config file %s",
 				  config_filename);

-- 
2.8.2.524.g6ff3d78

We could rely on atexit() to clean up everything, but let's be
explicit when we can. And it's good anyway because the function is
called the second time in the same process, we're in trouble.

This function should not affect the successful case because after
commit_lock_file() is called, rollback_lock_file() becomes no-op.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 config.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/config.c b/config.c
index 505e0d0..e02def4 100644
--- a/config.c
+++ b/config.c

@@ -2483,6 +2483,7 @@ int git_config_rename_section_in_file(const char *config_filename,
 		ret = error_errno("could not write config file %s",
 				  config_filename);
 out:
+	rollback_lock_file(lock);
 	free(filename_buf);
 	return ret;
 }

-- 
2.8.2.524.g6ff3d78