git/tempfile.h, branch jch

lockfile: report when rollback fails

2024-03-07T20:34:13Z

We do not report to the caller when rolling back a lockfile fails, which will be needed by the reftable compaction logic in a subsequent commit. It also cannot really report on all errors because the function calls `delete_tempfile()`, which doesn't return an error either. Refactor the code so that both `delete_tempfile()` and `rollback_lock_file()` return an error code. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano

tempfile: drop active flag

2022-08-30T21:16:49Z

Our tempfile struct contains an "active" flag. Long ago, this flag was important: tempfile structs were always allocated for the lifetime of the program and added to a global linked list, and the active flag was what told us whether a struct's tempfile needed to be cleaned up on exit. But since 422a21c6a0 (tempfile: remove deactivated list entries, 2017-09-05) and 076aa2cbda (tempfile: auto-allocate tempfiles on heap, 2017-09-05), we actually remove items from the list, and the active flag is generally always set to true for any allocated struct. We set it to true in all of the creation functions, and in the normal code flow it becomes false only in deactivate_tempfile(), which then immediately frees the struct. So the flag isn't performing that role anymore, and in fact makes things more confusing. Dscho noted that delete_tempfile() is a noop for an inactive struct. Since 076aa2cbda taught it to free the struct when deactivating, we'd leak any struct whose active flag is unset. But in practice it's not a leak, because again, we'll free when we unset the flag, and never see the allocated-but-inactive state. Can we just get rid of the flag? The answer is yes, but it requires looking at a few other spots: 1. I said above that the flag only becomes false before we deallocate, but there's one exception: when we call remove_tempfiles() from a signal or atexit handler, we unset the active flag as we remove each file. This isn't important for delete_tempfile(), as nobody would call it anymore, since we're exiting. It does in theory provide us some protection against racily double-removing a tempfile. If we receive a second signal while we are already in the cleanup routines, we'll start the cleanup loop again, and may visit the same tempfile. But this race already exists, because calling unlink() and unsetting the active flag aren't atomic! And it's OK in practice, because unlink() is idempotent (barring the unlikely event that some other process chooses our exact temp filename in that instant). So dropping the active flag widens the race a bit, but it was already there, and is fairly harmless in practice. If we really care about addressing it, the right thing is probably to block further signals while we're doing our cleanup (which we could actually do atomically). 2. The active flag is declared as "volatile sig_atomic_t". The idea is that it's the final bit that gets set to tell the cleanup routines that the tempfile is ready to be used (or not used), and it's safe to receive a signal racing with regular code which adds or removes a tempfile from the list. In practice, I don't think this is buying us anything. The presence on the linked list is really what tells the cleanup routines to look at the struct. That is already marked as "volatile". It's not a sig_atomic_t, so it's possible that we could see a sheared write there as an entry is added or removed. But that is true of the current code, too! Before we can even look at the "active" flag, we'd have to follow a link to the struct itself. If we see a sheared write in the pointer to the struct, then we'll look at garbage memory anyway, and there's not much we can do. This patch removes the active flag entirely, using presence on the global linked list as an indicator that a tempfile ought to be cleaned up. We are already careful to add to the list as the final step in activating. On deactivation, we'll make sure to remove from the list as the first step, before freeing any fields. The use of the volatile keyword should mean that those things happen in the expected order. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

tempfile: avoid directory cleanup race

2022-08-27T17:17:46Z

The temporary directory created by mks_tempfile_dt() is deleted by first deleting the file within, then truncating the filename strbuf and passing the resulting string to rmdir(2). When the cleanup routine is invoked concurrently by a signal handler we can end up passing the now truncated string to unlink(2), however, which could cause problems on some systems. Avoid that issue by remembering the directory name separately. This way the paths stay unchanged. A signal handler can still race with normal cleanup, but deleting the same files and directories twice is harmless. Reported-by: Jeff King Signed-off-by: René Scharfe Signed-off-by: Junio C Hamano

tempfile: add mks_tempfile_dt()

2022-04-20T23:17:33Z

Add a function to create a temporary file with a certain name in a temporary directory created using mkdtemp(3). Its result is more sightly than the paths created by mks_tempfile_ts(), which include a random prefix. That's useful for files passed to a program that displays their name, e.g. an external diff tool. Signed-off-by: René Scharfe Signed-off-by: Junio C Hamano

tempfile.c: introduce 'create_tempfile_mode'

2020-04-27T18:27:35Z

In the next patch, 'hold_lock_file_for_update' will gain an additional 'mode' parameter to specify permissions for the associated temporary file. Since the lockfile.c machinery uses 'create_tempfile' which always creates a temporary file with global read-write permissions, introduce a variant here that allows specifying the mode. Note that the mode given to 'create_tempfile_mode' is not guaranteed to be written to disk, since it is subject to both the umask and 'core.sharedRepository'. Arguably, all temporary files should have permission 0444, since they are likely to be renamed into place and then not written to again. This is a much larger change than we may want to take on in this otherwise small patch, so for the time being, make 'create_tempfile' behave as it has always done by inlining it to 'create_tempfile_mode' with mode set to '0666'. Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano

*.[ch]: manually align parameter lists

2019-05-05T06:20:10Z

In previous patches, extern was mechanically removed from function declarations without care to formatting, causing parameter lists to be misaligned. Manually format changed sections such that the parameter lists should be realigned. Viewing this patch with 'git diff -w' should produce no output. Signed-off-by: Denton Liu Signed-off-by: Junio C Hamano

*.[ch]: remove extern from function declarations using spatch

2019-05-05T06:20:06Z

There has been a push to remove extern from function declarations. Remove some instances of "extern" for function declarations which are caught by Coccinelle. Note that Coccinelle has some difficulty with processing functions with `__attribute__` or varargs so some `extern` declarations are left behind to be dealt with in a future patch. This was the Coccinelle patch used: @@ type T; identifier f; @@ - extern T f(...); and it was run with: $ git ls-files \*.{c,h} | grep -v ^compat/ | xargs spatch --sp-file contrib/coccinelle/noextern.cocci --in-place Files under `compat/` are intentionally excluded as some are directly copied from external sources and we should avoid churning them as much as possible. Signed-off-by: Denton Liu Signed-off-by: Junio C Hamano

Merge branch 'jk/reopen-tempfile-truncate'

2018-09-24T17:30:46Z

Fix for a long-standing bug that leaves the index file corrupt when it shrinks during a partial commit. * jk/reopen-tempfile-truncate: reopen_tempfile(): truncate opened file

reopen_tempfile(): truncate opened file

2018-09-05T16:46:16Z

We provide a reopen_tempfile() function, which is in turn used by reopen_lockfile(). The idea is that a caller may want to rewrite the tempfile without letting go of the lock. And that's what our one caller does: after running add--interactive, "commit -p" will update the cache-tree extension of the index and write out the result, all while holding the lock. However, because we open the file with only the O_WRONLY flag, the existing index content is left in place, and we overwrite it starting at position 0. If the new index after updating the cache-tree is smaller than the original, those final bytes are not overwritten and remain in the file. This results in a corrupt index, since those cruft bytes are interpreted as part of the trailing hash (or even as an extension, if there are enough bytes). This bug actually pre-dates reopen_tempfile(); the original code from 9c4d6c0297 (cache-tree: Write updated cache-tree after commit, 2014-07-13) has the same bug, and those lines were eventually refactored into the tempfile module. Nobody noticed until now for two reasons: - the bug can only be triggered in interactive mode ("commit -p" or "commit -i") - the size of the index must shrink after updating the cache-tree, which implies a non-trivial deletion. Notice that the included test actually has to create a 2-deep hierarchy. A single level is not enough to actually cause shrinkage. The fix is to truncate the file before writing out the second index. We can do that at the caller by using ftruncate(). But we shouldn't have to do that. There is no other place in Git where we want to open a file and overwrite bytes, making reopen_tempfile() a confusing and error-prone interface. Let's pass O_TRUNC there, which gives callers the same state they had after initially opening the file or lock. It's possible that we could later add a caller that wants something else (e.g., to open with O_APPEND). But this is the only caller we've had in the history of the codebase. Let's punt on doing anything more clever until another one comes along. Reported-by: Luc Van Oostenryck Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

Add missing includes and forward declarations

2018-08-15T18:52:09Z

I looped over the toplevel header files, creating a temporary two-line C program for each consisting of #include "git-compat-util.h" #include $HEADER This patch is the result of manually fixing errors in compiling those tiny programs. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano