[v2,1/3] pack-write.c: rename `.idx` files into place last

Commit Message

Taylor Blau Sept. 8, 2021, 10:17 p.m. UTC

We treat the presence of an `.idx` file as the indicator of whether or
not it's safe to use a packfile. But `finish_tmp_packfile()` (which is
responsible for writing the pack and moving the temporary versions of
all of its auxiliary files into place) is inconsistent about the write
order.

But the `.rev` file is moved into place after the `.idx`, so it's
possible for a process to open a pack in between the two (i.e., while
the `.idx` file is in place but the `.rev` file is not) and mistakenly
fall back to generating the pack's reverse index in memory. Though racy,
this amounts to an unnecessary slow-down at worst, and doesn't affect
the correctness of the resulting reverse index.

Close this race by moving the .rev file into place before moving the
.idx file into place.

While we're here, only call strbuf_setlen() if we actually modified the
buffer by bringing it inside of the same if-statement that calls
rename().

Signed-off-by: Taylor Blau <[e-mail]>
---
 pack-write.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Patch

diff --git a/pack-write.c b/pack-write.c
index f1fc3ecafa..277c60165e 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -490,18 +490,18 @@  void finish_tmp_packfile(struct strbuf *name_buffer,
 
 	strbuf_setlen(name_buffer, basename_len);
 
-	strbuf_addf(name_buffer, "%s.idx", hash_to_hex(hash));
-	if (rename(idx_tmp_name, name_buffer->buf))
-		die_errno("unable to rename temporary index file");
-
-	strbuf_setlen(name_buffer, basename_len);
-
 	if (rev_tmp_name) {
 		strbuf_addf(name_buffer, "%s.rev", hash_to_hex(hash));
 		if (rename(rev_tmp_name, name_buffer->buf))
 			die_errno("unable to rename temporary reverse-index file");
+
+		strbuf_setlen(name_buffer, basename_len);
 	}
 
+	strbuf_addf(name_buffer, "%s.idx", hash_to_hex(hash));
+	if (rename(idx_tmp_name, name_buffer->buf))
+		die_errno("unable to rename temporary index file");
+
 	strbuf_setlen(name_buffer, basename_len);
 
 	free((void *)idx_tmp_name);