Skip to content

Commit 076aa2c

Browse files
peffgitster
peff
authored and
gitster
committed
tempfile: auto-allocate tempfiles on heap
The previous commit taught the tempfile code to give up ownership over tempfiles that have been renamed or deleted. That makes it possible to use a stack variable like this: struct tempfile t; create_tempfile(&t, ...); ... if (!err) rename_tempfile(&t, ...); else delete_tempfile(&t); But doing it this way has a high potential for creating memory errors. The tempfile we pass to create_tempfile() ends up on a global linked list, and it's not safe for it to go out of scope until we've called one of those two deactivation functions. Imagine that we add an early return from the function that forgets to call delete_tempfile(). With a static or heap tempfile variable, the worst case is that the tempfile hangs around until the program exits (and some functions like setup_shallow_temporary rely on this intentionally, creating a tempfile and then leaving it for later cleanup). But with a stack variable as above, this is a serious memory error: the variable goes out of scope and may be filled with garbage by the time the tempfile code looks at it. Let's see if we can make it harder to get this wrong. Since many callers need to allocate arbitrary numbers of tempfiles, we can't rely on static storage as a general solution. So we need to turn to the heap. We could just ask all callers to pass us a heap variable, but that puts the burden on them to call free() at the right time. Instead, let's have the tempfile code handle the heap allocation _and_ the deallocation (when the tempfile is deactivated and removed from the list). This changes the return value of all of the creation functions. For the cleanup functions (delete and rename), we'll add one extra bit of safety: instead of taking a tempfile pointer, we'll take a pointer-to-pointer and set it to NULL after freeing the object. This makes it safe to double-call functions like delete_tempfile(), as the second call treats the NULL input as a noop. Several callsites follow this pattern. The resulting patch does have a fair bit of noise, as each caller needs to be converted to handle: 1. Storing a pointer instead of the struct itself. 2. Passing the pointer instead of taking the struct address. 3. Handling a "struct tempfile *" return instead of a file descriptor. We could play games to make this less noisy. For example, by defining the tempfile like this: struct tempfile { struct heap_allocated_part_of_tempfile { int fd; ...etc } *actual_data; } Callers would continue to have a "struct tempfile", and it would be "active" only when the inner pointer was non-NULL. But that just makes things more awkward in the long run. There aren't that many callers, so we can simply bite the bullet and adjust all of them. And the compiler makes it easy for us to find them all. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
1 parent 422a21c commit 076aa2c

File tree

13 files changed

+136
-141
lines changed

13 files changed

+136
-141
lines changed

‎builtin/gc.c‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ static struct argv_array prune = ARGV_ARRAY_INIT;
4747
static struct argv_array prune_worktrees = ARGV_ARRAY_INIT;
4848
static struct argv_array rerere = ARGV_ARRAY_INIT;
4949

50-
static struct tempfile pidfile;
50+
static struct tempfile *pidfile;
5151
static struct lock_file log_lock;
5252

5353
static struct string_list pack_garbage = STRING_LIST_INIT_DUP;
@@ -78,7 +78,7 @@ static void process_log_file(void)
7878
*/
7979
int saved_errno = errno;
8080
fprintf(stderr, _("Failed to fstat %s: %s"),
81-
get_tempfile_path(&log_lock.tempfile),
81+
get_tempfile_path(log_lock.tempfile),
8282
strerror(saved_errno));
8383
fflush(stderr);
8484
commit_lock_file(&log_lock);
@@ -242,7 +242,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
242242
int fd;
243243
char *pidfile_path;
244244

245-
if (is_tempfile_active(&pidfile))
245+
if (is_tempfile_active(pidfile))
246246
/* already locked */
247247
return NULL;
248248

@@ -293,7 +293,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
293293
write_in_full(fd, sb.buf, sb.len);
294294
strbuf_release(&sb);
295295
commit_lock_file(&lock);
296-
register_tempfile(&pidfile, pidfile_path);
296+
pidfile = register_tempfile(pidfile_path);
297297
free(pidfile_path);
298298
return NULL;
299299
}

‎credential-cache--daemon.c‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,6 @@
55
#include "unix-socket.h"
66
#include "parse-options.h"
77

8-
static struct tempfile socket_file;
9-
108
struct credential_cache_entry {
119
struct credential item;
1210
timestamp_t expiration;
@@ -260,6 +258,7 @@ static void init_socket_directory(const char *path)
260258

261259
int cmd_main(int argc, const char **argv)
262260
{
261+
struct tempfile *socket_file;
263262
const char *socket_path;
264263
int ignore_sighup = 0;
265264
static const char *usage[] = {
@@ -285,7 +284,7 @@ int cmd_main(int argc, const char **argv)
285284
die("socket directory must be an absolute path");
286285

287286
init_socket_directory(socket_path);
288-
register_tempfile(&socket_file, socket_path);
287+
socket_file = register_tempfile(socket_path);
289288

290289
if (ignore_sighup)
291290
signal(SIGHUP, SIG_IGN);

‎diff.c‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -459,7 +459,7 @@ static struct diff_tempfile {
459459
* If this diff_tempfile instance refers to a temporary file,
460460
* this tempfile object is used to manage its lifetime.
461461
*/
462-
struct tempfile tempfile;
462+
struct tempfile *tempfile;
463463
} diff_temp[2];
464464

465465
struct emit_callback {
@@ -1414,7 +1414,7 @@ static void remove_tempfile(void)
14141414
{
14151415
int i;
14161416
for (i = 0; i < ARRAY_SIZE(diff_temp); i++) {
1417-
if (is_tempfile_active(&diff_temp[i].tempfile))
1417+
if (is_tempfile_active(diff_temp[i].tempfile))
14181418
delete_tempfile(&diff_temp[i].tempfile);
14191419
diff_temp[i].name = NULL;
14201420
}
@@ -3720,7 +3720,6 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,
37203720
const struct object_id *oid,
37213721
int mode)
37223722
{
3723-
int fd;
37243723
struct strbuf buf = STRBUF_INIT;
37253724
struct strbuf template = STRBUF_INIT;
37263725
char *path_dup = xstrdup(path);
@@ -3730,18 +3729,18 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,
37303729
strbuf_addstr(&template, "XXXXXX_");
37313730
strbuf_addstr(&template, base);
37323731

3733-
fd = mks_tempfile_ts(&temp->tempfile, template.buf, strlen(base) + 1);
3734-
if (fd < 0)
3732+
temp->tempfile = mks_tempfile_ts(template.buf, strlen(base) + 1);
3733+
if (!temp->tempfile)
37353734
die_errno("unable to create temp-file");
37363735
if (convert_to_working_tree(path,
37373736
(const char *)blob, (size_t)size, &buf)) {
37383737
blob = buf.buf;
37393738
size = buf.len;
37403739
}
3741-
if (write_in_full(fd, blob, size) != size ||
3742-
close_tempfile_gently(&temp->tempfile))
3740+
if (write_in_full(temp->tempfile->fd, blob, size) != size ||
3741+
close_tempfile_gently(temp->tempfile))
37433742
die_errno("unable to write temp-file");
3744-
temp->name = get_tempfile_path(&temp->tempfile);
3743+
temp->name = get_tempfile_path(temp->tempfile);
37453744
oid_to_hex_r(temp->hex, oid);
37463745
xsnprintf(temp->mode, sizeof(temp->mode), "%06o", mode);
37473746
strbuf_release(&buf);

‎gpg-interface.c‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -202,17 +202,17 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
202202
struct strbuf *gpg_output, struct strbuf *gpg_status)
203203
{
204204
struct child_process gpg = CHILD_PROCESS_INIT;
205-
static struct tempfile temp;
206-
int fd, ret;
205+
struct tempfile *temp;
206+
int ret;
207207
struct strbuf buf = STRBUF_INIT;
208208

209-
fd = mks_tempfile_t(&temp, ".git_vtag_tmpXXXXXX");
210-
if (fd < 0)
209+
temp = mks_tempfile_t(".git_vtag_tmpXXXXXX");
210+
if (!temp)
211211
return error_errno(_("could not create temporary file"));
212-
if (write_in_full(fd, signature, signature_size) < 0 ||
213-
close_tempfile_gently(&temp) < 0) {
212+
if (write_in_full(temp->fd, signature, signature_size) < 0 ||
213+
close_tempfile_gently(temp) < 0) {
214214
error_errno(_("failed writing detached signature to '%s'"),
215-
temp.filename.buf);
215+
temp->filename.buf);
216216
delete_tempfile(&temp);
217217
return -1;
218218
}
@@ -221,7 +221,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
221221
gpg_program,
222222
"--status-fd=1",
223223
"--keyid-format=long",
224-
"--verify", temp.filename.buf, "-",
224+
"--verify", temp->filename.buf, "-",
225225
NULL);
226226

227227
if (!gpg_status)

‎lockfile.c‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -72,17 +72,16 @@ static void resolve_symlink(struct strbuf *path)
7272
/* Make sure errno contains a meaningful value on error */
7373
static int lock_file(struct lock_file *lk, const char *path, int flags)
7474
{
75-
int fd;
7675
struct strbuf filename = STRBUF_INIT;
7776

7877
strbuf_addstr(&filename, path);
7978
if (!(flags & LOCK_NO_DEREF))
8079
resolve_symlink(&filename);
8180

8281
strbuf_addstr(&filename, LOCK_SUFFIX);
83-
fd = create_tempfile(&lk->tempfile, filename.buf);
82+
lk->tempfile = create_tempfile(filename.buf);
8483
strbuf_release(&filename);
85-
return fd;
84+
return lk->tempfile ? lk->tempfile->fd : -1;
8685
}
8786

8887
/*
@@ -191,7 +190,7 @@ char *get_locked_file_path(struct lock_file *lk)
191190
{
192191
struct strbuf ret = STRBUF_INIT;
193192

194-
strbuf_addstr(&ret, get_tempfile_path(&lk->tempfile));
193+
strbuf_addstr(&ret, get_tempfile_path(lk->tempfile));
195194
if (ret.len <= LOCK_SUFFIX_LEN ||
196195
strcmp(ret.buf + ret.len - LOCK_SUFFIX_LEN, LOCK_SUFFIX))
197196
die("BUG: get_locked_file_path() called for malformed lock object");

‎lockfile.h‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@
111111
#include "tempfile.h"
112112

113113
struct lock_file {
114-
struct tempfile tempfile;
114+
struct tempfile *tempfile;
115115
};
116116

117117
/* String appended to a filename to derive the lockfile name: */
@@ -180,7 +180,7 @@ static inline int hold_lock_file_for_update(
180180
*/
181181
static inline int is_lock_file_locked(struct lock_file *lk)
182182
{
183-
return is_tempfile_active(&lk->tempfile);
183+
return is_tempfile_active(lk->tempfile);
184184
}
185185

186186
/*
@@ -208,7 +208,7 @@ extern NORETURN void unable_to_lock_die(const char *path, int err);
208208
*/
209209
static inline FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)
210210
{
211-
return fdopen_tempfile(&lk->tempfile, mode);
211+
return fdopen_tempfile(lk->tempfile, mode);
212212
}
213213

214214
/*
@@ -217,17 +217,17 @@ static inline FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)
217217
*/
218218
static inline const char *get_lock_file_path(struct lock_file *lk)
219219
{
220-
return get_tempfile_path(&lk->tempfile);
220+
return get_tempfile_path(lk->tempfile);
221221
}
222222

223223
static inline int get_lock_file_fd(struct lock_file *lk)
224224
{
225-
return get_tempfile_fd(&lk->tempfile);
225+
return get_tempfile_fd(lk->tempfile);
226226
}
227227

228228
static inline FILE *get_lock_file_fp(struct lock_file *lk)
229229
{
230-
return get_tempfile_fp(&lk->tempfile);
230+
return get_tempfile_fp(lk->tempfile);
231231
}
232232

233233
/*
@@ -246,7 +246,7 @@ extern char *get_locked_file_path(struct lock_file *lk);
246246
*/
247247
static inline int close_lock_file_gently(struct lock_file *lk)
248248
{
249-
return close_tempfile_gently(&lk->tempfile);
249+
return close_tempfile_gently(lk->tempfile);
250250
}
251251

252252
/*
@@ -270,7 +270,7 @@ static inline int close_lock_file_gently(struct lock_file *lk)
270270
*/
271271
static inline int reopen_lock_file(struct lock_file *lk)
272272
{
273-
return reopen_tempfile(&lk->tempfile);
273+
return reopen_tempfile(lk->tempfile);
274274
}
275275

276276
/*

‎read-cache.c‎

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2311,7 +2311,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
23112311
return -1;
23122312
if (close_tempfile_gently(tempfile)) {
23132313
error(_("could not close '%s'"), tempfile->filename.buf);
2314-
delete_tempfile(tempfile);
2314+
delete_tempfile(&tempfile);
23152315
return -1;
23162316
}
23172317
if (stat(tempfile->filename.buf, &st))
@@ -2337,7 +2337,7 @@ static int commit_locked_index(struct lock_file *lk)
23372337
static int do_write_locked_index(struct index_state *istate, struct lock_file *lock,
23382338
unsigned flags)
23392339
{
2340-
int ret = do_write_index(istate, &lock->tempfile, 0);
2340+
int ret = do_write_index(istate, lock->tempfile, 0);
23412341
if (ret)
23422342
return ret;
23432343
assert((flags & (COMMIT_LOCK | CLOSE_LOCK)) !=
@@ -2420,34 +2420,33 @@ static int clean_shared_index_files(const char *current_hex)
24202420
return 0;
24212421
}
24222422

2423-
static struct tempfile temporary_sharedindex;
2424-
24252423
static int write_shared_index(struct index_state *istate,
24262424
struct lock_file *lock, unsigned flags)
24272425
{
2426+
struct tempfile *temp;
24282427
struct split_index *si = istate->split_index;
2429-
int fd, ret;
2428+
int ret;
24302429

2431-
fd = mks_tempfile(&temporary_sharedindex, git_path("sharedindex_XXXXXX"));
2432-
if (fd < 0) {
2430+
temp = mks_tempfile(git_path("sharedindex_XXXXXX"));
2431+
if (!temp) {
24332432
hashclr(si->base_sha1);
24342433
return do_write_locked_index(istate, lock, flags);
24352434
}
24362435
move_cache_to_base_index(istate);
2437-
ret = do_write_index(si->base, &temporary_sharedindex, 1);
2436+
ret = do_write_index(si->base, temp, 1);
24382437
if (ret) {
2439-
delete_tempfile(&temporary_sharedindex);
2438+
delete_tempfile(&temp);
24402439
return ret;
24412440
}
2442-
ret = adjust_shared_perm(get_tempfile_path(&temporary_sharedindex));
2441+
ret = adjust_shared_perm(get_tempfile_path(temp));
24432442
if (ret) {
24442443
int save_errno = errno;
2445-
error("cannot fix permission bits on %s", get_tempfile_path(&temporary_sharedindex));
2446-
delete_tempfile(&temporary_sharedindex);
2444+
error("cannot fix permission bits on %s", get_tempfile_path(temp));
2445+
delete_tempfile(&temp);
24472446
errno = save_errno;
24482447
return ret;
24492448
}
2450-
ret = rename_tempfile(&temporary_sharedindex,
2449+
ret = rename_tempfile(&temp,
24512450
git_path("sharedindex.%s", sha1_to_hex(si->base->sha1)));
24522451
if (!ret) {
24532452
hashcpy(si->base_sha1, si->base->sha1);

‎refs/files-backend.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1747,12 +1747,12 @@ static int create_symref_locked(struct files_ref_store *refs,
17471747

17481748
if (!fdopen_lock_file(lock->lk, "w"))
17491749
return error("unable to fdopen %s: %s",
1750-
lock->lk->tempfile.filename.buf, strerror(errno));
1750+
lock->lk->tempfile->filename.buf, strerror(errno));
17511751

17521752
update_symref_reflog(refs, lock, refname, target, logmsg);
17531753

17541754
/* no error check; commit_ref will check ferror */
1755-
fprintf(lock->lk->tempfile.fp, "ref: %s\n", target);
1755+
fprintf(lock->lk->tempfile->fp, "ref: %s\n", target);
17561756
if (commit_ref(lock) < 0)
17571757
return error("unable to write symref for %s: %s", refname,
17581758
strerror(errno));

‎refs/packed-backend.c‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ struct packed_ref_store {
7575
* "packed-refs" file. Note that this (and thus the enclosing
7676
* `packed_ref_store`) must not be freed.
7777
*/
78-
struct tempfile tempfile;
78+
struct tempfile *tempfile;
7979
};
8080

8181
struct ref_store *packed_ref_store_create(const char *path,
@@ -628,15 +628,16 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
628628
*/
629629
packed_refs_path = get_locked_file_path(&refs->lock);
630630
strbuf_addf(&sb, "%s.new", packed_refs_path);
631-
if (create_tempfile(&refs->tempfile, sb.buf) < 0) {
631+
refs->tempfile = create_tempfile(sb.buf);
632+
if (!refs->tempfile) {
632633
strbuf_addf(err, "unable to create file %s: %s",
633634
sb.buf, strerror(errno));
634635
strbuf_release(&sb);
635636
goto out;
636637
}
637638
strbuf_release(&sb);
638639

639-
out = fdopen_tempfile(&refs->tempfile, "w");
640+
out = fdopen_tempfile(refs->tempfile, "w");
640641
if (!out) {
641642
strbuf_addf(err, "unable to fdopen packed-refs tempfile: %s",
642643
strerror(errno));
@@ -645,7 +646,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
645646

646647
if (fprintf(out, "%s", PACKED_REFS_HEADER) < 0) {
647648
strbuf_addf(err, "error writing to %s: %s",
648-
get_tempfile_path(&refs->tempfile), strerror(errno));
649+
get_tempfile_path(refs->tempfile), strerror(errno));
649650
goto error;
650651
}
651652

@@ -657,7 +658,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
657658
if (write_packed_entry(out, iter->refname, iter->oid->hash,
658659
peel_error ? NULL : peeled.hash)) {
659660
strbuf_addf(err, "error writing to %s: %s",
660-
get_tempfile_path(&refs->tempfile),
661+
get_tempfile_path(refs->tempfile),
661662
strerror(errno));
662663
ref_iterator_abort(iter);
663664
goto error;

‎shallow.c‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -288,19 +288,18 @@ int write_shallow_commits(struct strbuf *out, int use_pack_protocol,
288288

289289
const char *setup_temporary_shallow(const struct oid_array *extra)
290290
{
291-
static struct tempfile temp;
291+
struct tempfile *temp;
292292
struct strbuf sb = STRBUF_INIT;
293-
int fd;
294293

295294
if (write_shallow_commits(&sb, 0, extra)) {
296-
fd = xmks_tempfile(&temp, git_path("shallow_XXXXXX"));
295+
temp = xmks_tempfile(git_path("shallow_XXXXXX"));
297296

298-
if (write_in_full(fd, sb.buf, sb.len) != sb.len ||
299-
close_tempfile_gently(&temp) < 0)
297+
if (write_in_full(temp->fd, sb.buf, sb.len) != sb.len ||
298+
close_tempfile_gently(temp) < 0)
300299
die_errno("failed to write to %s",
301-
get_tempfile_path(&temp));
300+
get_tempfile_path(temp));
302301
strbuf_release(&sb);
303-
return get_tempfile_path(&temp);
302+
return get_tempfile_path(temp);
304303
}
305304
/*
306305
* is_repository_shallow() sees empty string as "no shallow

0 commit comments

Comments
 (0)