reopen_tempfile(): truncate opened file

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 <luc.vanoostenryck@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

4 files changed, 23 insertions, 5 deletions

diff --git a/lockfile.h b/lockfile.h
index f401c979f0..35403ccc0d 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -263,8 +263,8 @@ static inline int close_lock_file_gently(struct lock_file *lk)
  *   nobody else) to inspect the contents you wrote, while still
  *   holding the lock yourself.
  *
- * * `reopen_lock_file()` to reopen the lockfile. Make further updates
- *   to the contents.
+ * * `reopen_lock_file()` to reopen the lockfile, truncating the existing
+ *   contents. Write out the new contents.
  *
  * * `commit_lock_file()` to make the final version permanent.
  */
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 0c61268fd2..596e269394 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -161,6 +161,24 @@ test_expect_success PERL 'commit --interactive gives cache-tree on partial commi
 	test_cache_tree
 '
 
+test_expect_success PERL 'commit -p with shrinking cache-tree' '
+	mkdir -p deep/subdir &&
+	echo content >deep/subdir/file &&
+	git add deep &&
+	git commit -m add &&
+	git rm -r deep &&
+
+	before=$(wc -c <.git/index) &&
+	git commit -m delete -p &&
+	after=$(wc -c <.git/index) &&
+
+	# double check that the index shrank
+	test $before -gt $after &&
+
+	# and that our index was not corrupted
+	git fsck
+'
+
 test_expect_success 'commit in child dir has cache-tree' '
 	mkdir dir &&
 	>dir/child.t &&
diff --git a/tempfile.c b/tempfile.c
index 139ecd97f8..d43ad8c191 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -279,7 +279,7 @@ int reopen_tempfile(struct tempfile *tempfile)
 		BUG("reopen_tempfile called for an inactive object");
 	if (0 <= tempfile->fd)
 		BUG("reopen_tempfile called for an open object");
-	tempfile->fd = open(tempfile->filename.buf, O_WRONLY);
+	tempfile->fd = open(tempfile->filename.buf, O_WRONLY|O_TRUNC);
 	return tempfile->fd;
 }
 
diff --git a/tempfile.h b/tempfile.h
index 8959c5f1b5..b1b085540b 100644
--- a/tempfile.h
+++ b/tempfile.h
@@ -235,8 +235,8 @@ extern int close_tempfile_gently(struct tempfile *tempfile);
  *   it (and nobody else) to inspect or even modify the file's
  *   contents.
  *
- * * `reopen_tempfile()` to reopen the temporary file. Make further
- *   updates to the contents.
+ * * `reopen_tempfile()` to reopen the temporary file, truncating the existing
+ *   contents. Write out the new contents.
  *
  * * `rename_tempfile()` to move the file to its permanent location.
  */