index-pack: make pointer-alias fallbacks safer

The final() function accepts a NULL value for certain parameters, and falls back to writing into a reusable "name" buffer, and then either: 1. For "keep_name", requiring all uses to do "keep_name ? keep_name : name.buf". This is awkward, and it's easy to accidentally look at the maybe-NULL keep_name. 2. For "final_index_name" and "final_pack_name", aliasing those pointers to the "name" buffer. This is easier to use, but the aliased pointers become invalid after the buffer is reused (this isn't a bug now, but it's a potential pitfall). One way to make this safer would be to introduce an extra pointer to do the aliasing, and have its lifetime match the validity of the "name" buffer. But it's still easy to accidentally use the wrong name (i.e., to use "final_pack_name" instead of the aliased pointer). Instead, let's use three separate buffers that will remain valid through the function. That makes it safe to alias the pointers and use them consistently. The extra allocations shouldn't matter, as this function is not performance sensitive. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Mar 16, 2017 at 10:27 UTC f20754802a280c57a1e5886605b6805bbf040c63
1 file changed +12 -8
builtin/index-pack.c
+12 -8
@@ -1386,7 +1386,9 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
1386 unsigned char *sha1)
1387 {
1388 const char *report = "pack";
1389 - struct strbuf name = STRBUF_INIT;
1389 + struct strbuf pack_name = STRBUF_INIT;
1390 + struct strbuf index_name = STRBUF_INIT;
1391 + struct strbuf keep_name_buf = STRBUF_INIT;
1392 int err;
1393
1394 if (!from_stdin) {
@@ -1402,13 +1404,13 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
1404 int keep_fd, keep_msg_len = strlen(keep_msg);
1405
1406 if (!keep_name)
1405 - odb_pack_name(&name, sha1, "keep");
1407 + keep_name = odb_pack_name(&keep_name_buf, sha1, "keep");
1408
1407 - keep_fd = odb_pack_keep(keep_name ? keep_name : name.buf);
1409 + keep_fd = odb_pack_keep(keep_name);
1410 if (keep_fd < 0) {
1411 if (errno != EEXIST)
1412 die_errno(_("cannot write keep file '%s'"),
1411 - keep_name ? keep_name : name.buf);
1413 + keep_name);
1414 } else {
1415 if (keep_msg_len > 0) {
1416 write_or_die(keep_fd, keep_msg, keep_msg_len);
@@ -1416,14 +1418,14 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
1418 }
1419 if (close(keep_fd) != 0)
1420 die_errno(_("cannot close written keep file '%s'"),
1419 - keep_name ? keep_name : name.buf);
1421 + keep_name);
1422 report = "keep";
1423 }
1424 }
1425
1426 if (final_pack_name != curr_pack_name) {
1427 if (!final_pack_name)
1426 - final_pack_name = odb_pack_name(&name, sha1, "pack");
1428 + final_pack_name = odb_pack_name(&pack_name, sha1, "pack");
1429 if (finalize_object_file(curr_pack_name, final_pack_name))
1430 die(_("cannot store pack file"));
1431 } else if (from_stdin)
@@ -1431,7 +1433,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
1433
1434 if (final_index_name != curr_index_name) {
1435 if (!final_index_name)
1434 - final_index_name = odb_pack_name(&name, sha1, "idx");
1436 + final_index_name = odb_pack_name(&index_name, sha1, "idx");
1437 if (finalize_object_file(curr_index_name, final_index_name))
1438 die(_("cannot store index file"));
1439 } else
@@ -1458,7 +1460,9 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
1460 }
1461 }
1462
1461 - strbuf_release(&name);
1463 + strbuf_release(&index_name);
1464 + strbuf_release(&pack_name);
1465 + strbuf_release(&keep_name_buf);
1466 }
1467
1468 static int git_index_pack_config(const char *k, const char *v, void *cb)