Segfault in git_config_set_multivar_in_file_gently with direct_io in FUSE filesystem
I am using git with a simple in-memory FUSE filesystem. When I enable direct_io, I get a segfault from git_config_set_multivar_in_file_gently during git clone.
I have full reproduction instructions using Go and macOS at
https://github.com/josharian/gitbug. It also includes a stack trace in case anyone wants to try blind debugging. Happy to provide more info if it will help.
Thanks,
Josh
PATCH 1/1 config.c: handle error case for fstat() calls
---
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.
config.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
config.c +10 -2
@@ -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",
Signature · 1 line
2.8.2.524.g6ff3d78
Re: [PATCH] config.c: handle error case for fstat() calls
Quoted 4 lines · Nguyễn Thái Ngọc Duy
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>---
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.
Quoted 33 lines
config.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)config.c +10
-2@@ -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",
Re: [PATCH] config.c: handle error case for fstat() calls
Quoted 5 lines · Junio C Hamano
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
PATCH 1/2 config.c: rename label unlock_and_out
There are two ways to unlock a file: commit, or revert. Rename it to commit_and_out to avoid confusion.
--- config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
config.c +2 -2
@@ -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);
Signature · 1 line
2.8.2.524.g6ff3d78
PATCH 2/2 config.c: handle lock file in error case in git_config_rename_...
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.
--- config.c | 1 + 1 file changed, 1 insertion(+)
config.c +1 -0
@@ -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; }
Signature · 1 line
2.8.2.524.g6ff3d78
Re: [PATCH 2/2] config.c: handle lock file in error case in git_config_rename_...
Quoted 6 lines · Nguyễn Thái Ngọc Duy
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.
Not really. At the point of the first "goto out" in this function, lock is merely an uninitialized pointer.
Quoted 20 lines
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>---
config.c | 1 + 1 file changed, 1 insertion(+)config.c +1
-0@@ -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; }