pack-write: fix return parameter of `write_rev_file_order()`
While the return parameter of `write_rev_file_order()` is a string constant, the function may indeed return an allocated string when its first parameter is a `NULL` pointer. This makes for a confusing calling convention, where callers need to be aware of these intricate ownership rules and cast away the constness to free the string in some cases. Adapt the function and its caller `write_rev_file()` to always return an allocated string and adapt callers to always free the return value. Note that this requires us to also adapt `rename_tmp_packfile()`, which compares the pointers to packfile data with each other. Now that the path of the reverse index file gets allocated unconditionally the check will always fail. This is fixed by using strcmp(3P) instead, which also feels way less fragile. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Patrick Steinhardt committed
Sep 30, 2024 at 11:14 UTC
2f0ee051ddaf880daac06773b56f077c4012a1c7
5 files changed
+31
-26
builtin/index-pack.c
+3
-4
@@ -1505,7 +1505,7 @@ static void rename_tmp_packfile(const char **final_name,
1505
struct strbuf *name, unsigned char *hash,
1506
const char *ext, int make_read_only_if_same)
1507
{
1508
- if (*final_name != curr_name) {
1508
+ if (!*final_name || strcmp(*final_name, curr_name)) {
1509
if (!*final_name)
1510
*final_name = odb_pack_name(name, hash, ext);
1511
if (finalize_object_file(curr_name, *final_name))
@@ -1726,7 +1726,7 @@ int cmd_index_pack(int argc,
1726
{
1727
int i, fix_thin_pack = 0, verify = 0, stat_only = 0, rev_index;
1728
const char *curr_index;
1729
- const char *curr_rev_index = NULL;
1729
+ char *curr_rev_index = NULL;
1730
const char *index_name = NULL, *pack_name = NULL, *rev_index_name = NULL;
1731
const char *keep_msg = NULL;
1732
const char *promisor_msg = NULL;
@@ -1968,8 +1968,7 @@ int cmd_index_pack(int argc,
1968
free((void *) curr_pack);
1969
if (!index_name)
1970
free((void *) curr_index);
1971
- if (!rev_index_name)
1972
- free((void *) curr_rev_index);
1971
+ free(curr_rev_index);
1972
1973
/*
1974
* Let the caller know this pack is not self contained
midx-write.c
+2
-1
@@ -649,7 +649,7 @@ static void write_midx_reverse_index(char *midx_name, unsigned char *midx_hash,
649
struct write_midx_context *ctx)
650
{
651
struct strbuf buf = STRBUF_INIT;
652
- const char *tmp_file;
652
+ char *tmp_file;
653
654
trace2_region_enter("midx", "write_midx_reverse_index", the_repository);
655
@@ -662,6 +662,7 @@ static void write_midx_reverse_index(char *midx_name, unsigned char *midx_hash,
662
die(_("cannot store reverse index file"));
663
664
strbuf_release(&buf);
665
+ free(tmp_file);
666
667
trace2_region_leave("midx", "write_midx_reverse_index", the_repository);
668
}
pack-write.c
+23
-19
@@ -212,15 +212,15 @@ static void write_rev_trailer(struct hashfile *f, const unsigned char *hash)
212
hashwrite(f, hash, the_hash_algo->rawsz);
213
}
214
215
-const char *write_rev_file(const char *rev_name,
216
- struct pack_idx_entry **objects,
217
- uint32_t nr_objects,
218
- const unsigned char *hash,
219
- unsigned flags)
215
+char *write_rev_file(const char *rev_name,
216
+ struct pack_idx_entry **objects,
217
+ uint32_t nr_objects,
218
+ const unsigned char *hash,
219
+ unsigned flags)
220
{
221
uint32_t *pack_order;
222
uint32_t i;
223
- const char *ret;
223
+ char *ret;
224
225
if (!(flags & WRITE_REV) && !(flags & WRITE_REV_VERIFY))
226
return NULL;
@@ -238,13 +238,14 @@ const char *write_rev_file(const char *rev_name,
238
return ret;
239
}
240
241
-const char *write_rev_file_order(const char *rev_name,
242
- uint32_t *pack_order,
243
- uint32_t nr_objects,
244
- const unsigned char *hash,
245
- unsigned flags)
241
+char *write_rev_file_order(const char *rev_name,
242
+ uint32_t *pack_order,
243
+ uint32_t nr_objects,
244
+ const unsigned char *hash,
245
+ unsigned flags)
246
{
247
struct hashfile *f;
248
+ char *path;
249
int fd;
250
251
if ((flags & WRITE_REV) && (flags & WRITE_REV_VERIFY))
@@ -254,12 +255,13 @@ const char *write_rev_file_order(const char *rev_name,
255
if (!rev_name) {
256
struct strbuf tmp_file = STRBUF_INIT;
257
fd = odb_mkstemp(&tmp_file, "pack/tmp_rev_XXXXXX");
257
- rev_name = strbuf_detach(&tmp_file, NULL);
258
+ path = strbuf_detach(&tmp_file, NULL);
259
} else {
260
unlink(rev_name);
261
fd = xopen(rev_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
262
+ path = xstrdup(rev_name);
263
}
262
- f = hashfd(fd, rev_name);
264
+ f = hashfd(fd, path);
265
} else if (flags & WRITE_REV_VERIFY) {
266
struct stat statbuf;
267
if (stat(rev_name, &statbuf)) {
@@ -270,22 +272,24 @@ const char *write_rev_file_order(const char *rev_name,
272
die_errno(_("could not stat: %s"), rev_name);
273
}
274
f = hashfd_check(rev_name);
273
- } else
275
+ path = xstrdup(rev_name);
276
+ } else {
277
return NULL;
278
+ }
279
280
write_rev_header(f);
281
282
write_rev_index_positions(f, pack_order, nr_objects);
283
write_rev_trailer(f, hash);
284
281
- if (rev_name && adjust_shared_perm(rev_name) < 0)
282
- die(_("failed to make %s readable"), rev_name);
285
+ if (adjust_shared_perm(path) < 0)
286
+ die(_("failed to make %s readable"), path);
287
288
finalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA,
289
CSUM_HASH_IN_STREAM | CSUM_CLOSE |
290
((flags & WRITE_IDX_VERIFY) ? 0 : CSUM_FSYNC));
291
288
- return rev_name;
292
+ return path;
293
}
294
295
static void write_mtimes_header(struct hashfile *f)
@@ -549,7 +553,7 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,
553
unsigned char hash[],
554
char **idx_tmp_name)
555
{
552
- const char *rev_tmp_name = NULL;
556
+ char *rev_tmp_name = NULL;
557
char *mtimes_tmp_name = NULL;
558
559
if (adjust_shared_perm(pack_tmp_name))
@@ -575,7 +579,7 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,
579
if (mtimes_tmp_name)
580
rename_tmp_packfile(name_buffer, mtimes_tmp_name, "mtimes");
581
578
- free((char *)rev_tmp_name);
582
+ free(rev_tmp_name);
583
free(mtimes_tmp_name);
584
}
585
pack.h
+2
-2
@@ -96,8 +96,8 @@ struct ref;
96
97
void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought);
98
99
-const char *write_rev_file(const char *rev_name, struct pack_idx_entry **objects, uint32_t nr_objects, const unsigned char *hash, unsigned flags);
100
-const char *write_rev_file_order(const char *rev_name, uint32_t *pack_order, uint32_t nr_objects, const unsigned char *hash, unsigned flags);
99
+char *write_rev_file(const char *rev_name, struct pack_idx_entry **objects, uint32_t nr_objects, const unsigned char *hash, unsigned flags);
100
+char *write_rev_file_order(const char *rev_name, uint32_t *pack_order, uint32_t nr_objects, const unsigned char *hash, unsigned flags);
101
102
/*
103
* The "hdr" output buffer should be at least this big, which will handle sizes
t/t5327-multi-pack-bitmaps-rev.sh
+1
@@ -2,6 +2,7 @@
2
3
test_description='exercise basic multi-pack bitmap functionality (.rev files)'
4
5
+TEST_PASSES_SANITIZE_LEAK=true
6
. ./test-lib.sh
7
. "${TEST_DIRECTORY}/lib-bitmap.sh"
8