Fix pack file permission issue on NFS - #5935
pfitzseb wants to merge 2 commits into
Conversation
Sorry, something went wrong.
|
can you give this patch a quick try? #5556 (comment) if this works, we won't need to even involve chmod(2) and still keep the packfiles read-only in all platforms that don't have this issue. |
Sorry, something went wrong.
|
Yep, that works fine. You wanna take it from here or should I update this PR? |
Sorry, something went wrong.
your choice! |
Sorry, something went wrong.
Co-authored-by: Co-authored-by: lhchavez <lhchavez@lhchavez.com>
|
Ok, let me know what else needs doing here :) Not sure how to go about writing a test for this though... |
Sorry, something went wrong.
There was a problem hiding this comment.
Ok, let me know what else needs doing here :) Not sure how to go about writing a test for this though...
I myself don't know how to test this, haha. The current set of tests pass for all platforms, which means that the patched code is still POSIX-compliant, which probably means that the change is safe-ish.
We can't quite write a unit test for this (to prevent regressions) since it requires the underlying filesystem to be NFS. maybe having a nightly test for this is the right move (create a new Docker container based on the focal one where both the checkout and /tmp are localhost NFS-mounted directories [edit: maybe only /tmp needs to be. all tests create working directories there]), but that's out of the scope of this PR.
If you're interested in helping with the NFS nightly test, we can help in the Slack.
Sorry, something went wrong.
Co-authored-by: lhchavez <lhchavez@lhchavez.com>
|
I think it would be better to define a GIT_TMP_MAX with a specific value rather than loop on TMP_MAX here. According to linux.die.net some systems have a TMP_MAX as low as 26 since they mainly use the pid for the "random" name and that is the reason to why mktemp() shouldn't be used. OpenGroup says that "On XSI-conformant systems, the value of {TMP_MAX} is at least 10000." but otherwise it can be as low as 25. |
Sorry, something went wrong.
The current tests doesn't test that the library itself is POSIX-compliant, they test that the library works on some POSIX-comliant systems. This particular case is problematic since OpenGroup says:
So part of the problem is that there are filesystems that aren't POSIX compliant and we want to support them. I think older versions stated that the state of open filedescriptors are undefined after chmod. It could be that fchmod() on the opened filedescriptor works too, it would be a slightly smaller change. |
Sorry, something went wrong.
Ok, turns out I didn't test the current implementation properly. I does not work, while creating the pack file with 644 prems does (#5556 (comment)). I also tried diff --git a/src/futils.c b/src/futils.c
index a44820875..2934a3037 100644
--- a/src/futils.c
+++ b/src/futils.c
@@ -41,7 +41,7 @@ int git_futils_mktmp(git_buf *path_out, const char *filename, mode_t mode)
return -1;
}
- if (p_chmod(path_out->ptr, (mode & ~mask))) {
+ if (p_fchmod(fd, (mode & ~mask))) {
git_error_set(GIT_ERROR_OS,
"failed to set permissions on file '%s'", path_out->ptr);
return -1;
diff --git a/src/unix/posix.h b/src/unix/posix.h
index 7b3325e78..dd5424a22 100644
--- a/src/unix/posix.h
+++ b/src/unix/posix.h
@@ -75,8 +75,10 @@ GIT_INLINE(int) p_fsync(int fd)
*/
#ifdef __ANDROID__
# define p_chmod(p,m) 0
+# define p_fchmod(p,m) 0
#else
# define p_chmod(p,m) chmod(p, m)
+# define p_fchmod(p,m) fchmod(p, m)
#endif
/* see win32/posix.h for explanation about why this exists */, but that doesn't do the trick either. |
Sorry, something went wrong.
|
Maybe a solution would be to move the chmod from git_futils_mktmp and call it after the files are closed in git_filebuf_commit() and git_indexer_commit()? diff --git a/src/filebuf.c b/src/filebuf.c
index a3f6b1483..90bf7196d 100644
--- a/src/filebuf.c
+++ b/src/filebuf.c
@@ -294,6 +294,7 @@ int git_filebuf_open_withsize(git_filebuf *file, const char *path, int flags, mo
file->buf_pos = 0;
file->fd = -1;
file->last_error = BUFERR_OK;
+ file->mode = mode;
/* Allocate the main cache buffer */
if (!file->do_not_buffer) {
@@ -335,7 +336,7 @@ int git_filebuf_open_withsize(git_filebuf *file, const char *path, int flags, mo
git_buf tmp_path = GIT_BUF_INIT;
/* Open the file as temporary for locking */
- file->fd = git_futils_mktmp(&tmp_path, path, mode);
+ file->fd = git_futils_mktmp(&tmp_path, path);
if (file->fd < 0) {
git_buf_dispose(&tmp_path);
@@ -373,7 +374,7 @@ int git_filebuf_open_withsize(git_filebuf *file, const char *path, int flags, mo
}
/* open the file for locking */
- if ((error = lock_file(file, flags, mode)) < 0)
+ if ((error = lock_file(file, flags, 0666)) < 0)
goto cleanup;
file->created_lock = true;
@@ -415,6 +416,8 @@ int git_filebuf_commit_at(git_filebuf *file, const char *path)
int git_filebuf_commit(git_filebuf *file)
{
+ mode_t mask;
+
/* temporary files cannot be committed */
GIT_ASSERT_ARG(file);
GIT_ASSERT(file->path_original);
@@ -439,6 +442,13 @@ int git_filebuf_commit(git_filebuf *file)
file->fd = -1;
+ p_umask(mask = p_umask(0));
+ if (p_chmod(file->path_lock, (file->mode & ~mask))) {
+ git_error_set(GIT_ERROR_OS,
+ "failed to set permissions on file '%s'", file->path_lock);
+ goto on_error;
+ }
+
if (p_rename(file->path_lock, file->path_original) < 0) {
git_error_set(GIT_ERROR_OS, "failed to rename lockfile to '%s'", file->path_original);
goto on_error;
diff --git a/src/filebuf.h b/src/filebuf.h
index 9d53bc307..096b98681 100644
--- a/src/filebuf.h
+++ b/src/filebuf.h
@@ -46,6 +46,7 @@ struct git_filebuf {
size_t buf_size, buf_pos;
git_file fd;
+ mode_t mode;
bool fd_is_open;
bool created_lock;
bool did_rename;
diff --git a/src/futils.c b/src/futils.c
index a44820875..8bdd6e431 100644
--- a/src/futils.c
+++ b/src/futils.c
@@ -22,12 +22,9 @@ int git_futils_mkpath2file(const char *file_path, const mode_t mode)
GIT_MKDIR_PATH | GIT_MKDIR_SKIP_LAST | GIT_MKDIR_VERIFY_DIR);
}
-int git_futils_mktmp(git_buf *path_out, const char *filename, mode_t mode)
+int git_futils_mktmp(git_buf *path_out, const char *filename)
{
int fd;
- mode_t mask;
-
- p_umask(mask = p_umask(0));
git_buf_sets(path_out, filename);
git_buf_puts(path_out, "_git2_XXXXXX");
@@ -41,12 +38,6 @@ int git_futils_mktmp(git_buf *path_out, const char *filename, mode_t mode)
return -1;
}
- if (p_chmod(path_out->ptr, (mode & ~mask))) {
- git_error_set(GIT_ERROR_OS,
- "failed to set permissions on file '%s'", path_out->ptr);
- return -1;
- }
-
return fd;
}
diff --git a/src/futils.h b/src/futils.h
index 4668d7b63..cb475e589 100644
--- a/src/futils.h
+++ b/src/futils.h
@@ -174,7 +174,7 @@ extern int git_futils_rmdir_r(const char *path, const char *base, uint32_t flags
* Writes the filename into path_out.
* @return On success, an open file descriptor, else an error code < 0.
*/
-extern int git_futils_mktmp(git_buf *path_out, const char *filename, mode_t mode);
+extern int git_futils_mktmp(git_buf *path_out, const char *filename);
/**
* Move a file on the filesystem, create the
diff --git a/src/indexer.c b/src/indexer.c
index d546888cc..c6b4406b9 100644
--- a/src/indexer.c
+++ b/src/indexer.c
@@ -166,7 +166,7 @@ int git_indexer_new(
if (error < 0)
goto cleanup;
- fd = git_futils_mktmp(&tmp_path, git_buf_cstr(&path), idx->mode);
+ fd = git_futils_mktmp(&tmp_path, git_buf_cstr(&path));
git_buf_dispose(&path);
if (fd < 0)
goto cleanup;
@@ -1131,6 +1131,7 @@ int git_indexer_commit(git_indexer *idx, git_indexer_progress *stats)
git_oid trailer_hash, file_hash;
git_filebuf index_file = {0};
void *packfile_trailer;
+ mode_t mask;
if (!idx->parsed_header) {
git_error_set(GIT_ERROR_INDEXER, "incomplete pack header");
@@ -1298,6 +1299,14 @@ int git_indexer_commit(git_indexer *idx, git_indexer_progress *stats)
idx->pack->mwf.fd = -1;
+ /* Once the file is closed it is safe to set protection mode */
+ p_umask(mask = p_umask(0));
+ if (p_chmod(idx->pack->pack_name, (idx->mode & ~mask))) {
+ git_error_set(GIT_ERROR_OS,
+ "failed to set permissions on file '%s'", idx->pack->pack_name);
+ goto on_error;
+ }
+
if (index_path(&filename, idx, ".pack") < 0)
goto on_error;
diff --git a/src/path.c b/src/path.c
index 8928e49b8..7822b3cfc 100644
--- a/src/path.c
+++ b/src/path.c
@@ -2003,7 +2003,7 @@ bool git_path_supports_symlinks(const char *dir)
struct stat st;
int fd;
- if ((fd = git_futils_mktmp(&path, dir, 0666)) < 0 ||
+ if ((fd = git_futils_mktmp(&path, dir)) < 0 ||
p_close(fd) < 0 ||
p_unlink(path.ptr) < 0 ||
p_symlink("testing", path.ptr) < 0 ||
diff --git a/src/refdb_fs.c b/src/refdb_fs.c
index 0b8e103c2..3860ae832 100644
--- a/src/refdb_fs.c
+++ b/src/refdb_fs.c
@@ -2009,6 +2009,7 @@ static int refdb_reflog_fs__rename(git_refdb_backend *_backend, const char *old_
git_buf normalized = GIT_BUF_INIT;
git_repository *repo;
refdb_fs_backend *backend;
+ mode_t mask;
GIT_ASSERT_ARG(_backend);
GIT_ASSERT_ARG(old_name);
@@ -2045,13 +2046,21 @@ static int refdb_reflog_fs__rename(git_refdb_backend *_backend, const char *old_
if ((error = loose_path(&temp_path, git_buf_cstr(&temp_path), "temp_reflog")) < 0)
return error;
- if ((fd = git_futils_mktmp(&temp_path, git_buf_cstr(&temp_path), GIT_REFLOG_FILE_MODE)) < 0) {
+ if ((fd = git_futils_mktmp(&temp_path, git_buf_cstr(&temp_path))) < 0) {
error = -1;
goto cleanup;
}
p_close(fd);
+ p_umask(mask = p_umask(0));
+ if (p_chmod(&temp_path, (GIT_REFLOG_FILE_MODE & ~mask))) {
+ git_error_set(GIT_ERROR_OS,
+ "failed to set permissions on file '%s'", &temp_path);
+ error = -1;
+ goto cleanup;
+ }
+
if (p_rename(git_buf_cstr(&old_path), git_buf_cstr(&temp_path)) < 0) {
git_error_set(GIT_ERROR_OS, "failed to rename reflog for %s", new_name);
error = -1;
|
Sorry, something went wrong.
|
Yeah, I can confirm that that patch fixes the issue. |
Sorry, something went wrong.
|
It is mainly aimed at not doing chmod on open file descriptors, something that the patch from @lhchavez also takes care of. Not being able to truncate a write protected file while still being able to write to it with the open file descriptor seems more to be a bug in the NFS driver and would probably be better to fix on that end. On the side of libgit I think the best solution would have been a configuration variable like core.symlinks that says if the filesystem supports file modification on write protected files so that the file can be opened with p_open as a write protected file where it is supported or be write protected after closing otherwise. Out of curiosity, how does regular git handle NFS? |
Sorry, something went wrong.
This is a great question. Although loose objects and packfiles are 0444 by convention, is this a necessary thing? Should we just best-effort try to set it 0444 and fallback if that fails? |
Sorry, something went wrong.
From what I could get from the problem description it would probably not be a good solution for NFS. A best effort attempt to lock the file would however solve an issue I have on AmigaOS3.x, but so would the patch from @lhchavez The NFS issue could probably be solved if the indexer didn't write more data than necessary and didn't need the truncate to begin with, but I don't know enough about how the paging works to know if that is easy to implement. |
Sorry, something went wrong.
|
So what's the way forward here? I'm afraid I'm too unfamiliar with all of this to make any sort of decision on which is the right fix. |
Sorry, something went wrong.
It sounds to me like you have ran into a bug in the NFS server cd /localdir touch file chmod a-w file cp file /nfsdir A possible workaround could be to check if the ftruncate fails and in that case temporarily unlock the file for the ftruncate and try again, maybe with a disclaimer that it is a workaround to handle NFS. |
Sorry, something went wrong.
|
This was probably fixed by #6039, Did that one fix the problem for @pfitzseb too? Would it still be of interest to apply #5556 (comment) ? |
Sorry, something went wrong.
|
Looks like this was fixed in #6039 - let me know if not. |
Sorry, something went wrong.
This fixes #5556 by simply applying the patch in #5556 (comment).
I have absolutely no idea whether this is the right fix for the issue; if it is I'd be happy to figure out how to add a test and whatnot ;)