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>

Jeff King committed Sep 5, 2017 at 08:15 UTC 076aa2cbda5782426c45cd65017b81d77876297a
13 files changed +136 -141
builtin/gc.c
+4 -4
@@ -47,7 +47,7 @@ static struct argv_array prune = ARGV_ARRAY_INIT;
47 static struct argv_array prune_worktrees = ARGV_ARRAY_INIT;
48 static struct argv_array rerere = ARGV_ARRAY_INIT;
49
50 -static struct tempfile pidfile;
50 +static struct tempfile *pidfile;
51 static struct lock_file log_lock;
52
53 static struct string_list pack_garbage = STRING_LIST_INIT_DUP;
@@ -78,7 +78,7 @@ static void process_log_file(void)
78 */
79 int saved_errno = errno;
80 fprintf(stderr, _("Failed to fstat %s: %s"),
81 - get_tempfile_path(&log_lock.tempfile),
81 + get_tempfile_path(log_lock.tempfile),
82 strerror(saved_errno));
83 fflush(stderr);
84 commit_lock_file(&log_lock);
@@ -242,7 +242,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
242 int fd;
243 char *pidfile_path;
244
245 - if (is_tempfile_active(&pidfile))
245 + if (is_tempfile_active(pidfile))
246 /* already locked */
247 return NULL;
248
@@ -293,7 +293,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
293 write_in_full(fd, sb.buf, sb.len);
294 strbuf_release(&sb);
295 commit_lock_file(&lock);
296 - register_tempfile(&pidfile, pidfile_path);
296 + pidfile = register_tempfile(pidfile_path);
297 free(pidfile_path);
298 return NULL;
299 }
credential-cache--daemon.c
+2 -3
@@ -5,8 +5,6 @@
5 #include "unix-socket.h"
6 #include "parse-options.h"
7
8 -static struct tempfile socket_file;
9 -
8 struct credential_cache_entry {
9 struct credential item;
10 timestamp_t expiration;
@@ -260,6 +258,7 @@ static void init_socket_directory(const char *path)
258
259 int cmd_main(int argc, const char **argv)
260 {
261 + struct tempfile *socket_file;
262 const char *socket_path;
263 int ignore_sighup = 0;
264 static const char *usage[] = {
@@ -285,7 +284,7 @@ int cmd_main(int argc, const char **argv)
284 die("socket directory must be an absolute path");
285
286 init_socket_directory(socket_path);
288 - register_tempfile(&socket_file, socket_path);
287 + socket_file = register_tempfile(socket_path);
288
289 if (ignore_sighup)
290 signal(SIGHUP, SIG_IGN);
diff.c
+7 -8
@@ -459,7 +459,7 @@ static struct diff_tempfile {
459 * If this diff_tempfile instance refers to a temporary file,
460 * this tempfile object is used to manage its lifetime.
461 */
462 - struct tempfile tempfile;
462 + struct tempfile *tempfile;
463 } diff_temp[2];
464
465 struct emit_callback {
@@ -1414,7 +1414,7 @@ static void remove_tempfile(void)
1414 {
1415 int i;
1416 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))
1418 delete_tempfile(&diff_temp[i].tempfile);
1419 diff_temp[i].name = NULL;
1420 }
@@ -3720,7 +3720,6 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,
3720 const struct object_id *oid,
3721 int mode)
3722 {
3723 - int fd;
3723 struct strbuf buf = STRBUF_INIT;
3724 struct strbuf template = STRBUF_INIT;
3725 char *path_dup = xstrdup(path);
@@ -3730,18 +3729,18 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,
3729 strbuf_addstr(&template, "XXXXXX_");
3730 strbuf_addstr(&template, base);
3731
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)
3734 die_errno("unable to create temp-file");
3735 if (convert_to_working_tree(path,
3736 (const char *)blob, (size_t)size, &buf)) {
3737 blob = buf.buf;
3738 size = buf.len;
3739 }
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))
3742 die_errno("unable to write temp-file");
3744 - temp->name = get_tempfile_path(&temp->tempfile);
3743 + temp->name = get_tempfile_path(temp->tempfile);
3744 oid_to_hex_r(temp->hex, oid);
3745 xsnprintf(temp->mode, sizeof(temp->mode), "%06o", mode);
3746 strbuf_release(&buf);
gpg-interface.c
+8 -8
@@ -202,17 +202,17 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
202 struct strbuf *gpg_output, struct strbuf *gpg_status)
203 {
204 struct child_process gpg = CHILD_PROCESS_INIT;
205 - static struct tempfile temp;
206 - int fd, ret;
205 + struct tempfile *temp;
206 + int ret;
207 struct strbuf buf = STRBUF_INIT;
208
209 - fd = mks_tempfile_t(&temp, ".git_vtag_tmpXXXXXX");
210 - if (fd < 0)
209 + temp = mks_tempfile_t(".git_vtag_tmpXXXXXX");
210 + if (!temp)
211 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) {
214 error_errno(_("failed writing detached signature to '%s'"),
215 - temp.filename.buf);
215 + temp->filename.buf);
216 delete_tempfile(&temp);
217 return -1;
218 }
@@ -221,7 +221,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
221 gpg_program,
222 "--status-fd=1",
223 "--keyid-format=long",
224 - "--verify", temp.filename.buf, "-",
224 + "--verify", temp->filename.buf, "-",
225 NULL);
226
227 if (!gpg_status)
lockfile.c
+3 -4
@@ -72,7 +72,6 @@ static void resolve_symlink(struct strbuf *path)
72 /* Make sure errno contains a meaningful value on error */
73 static int lock_file(struct lock_file *lk, const char *path, int flags)
74 {
75 - int fd;
75 struct strbuf filename = STRBUF_INIT;
76
77 strbuf_addstr(&filename, path);
@@ -80,9 +79,9 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
79 resolve_symlink(&filename);
80
81 strbuf_addstr(&filename, LOCK_SUFFIX);
83 - fd = create_tempfile(&lk->tempfile, filename.buf);
82 + lk->tempfile = create_tempfile(filename.buf);
83 strbuf_release(&filename);
85 - return fd;
84 + return lk->tempfile ? lk->tempfile->fd : -1;
85 }
86
87 /*
@@ -191,7 +190,7 @@ char *get_locked_file_path(struct lock_file *lk)
190 {
191 struct strbuf ret = STRBUF_INIT;
192
194 - strbuf_addstr(&ret, get_tempfile_path(&lk->tempfile));
193 + strbuf_addstr(&ret, get_tempfile_path(lk->tempfile));
194 if (ret.len <= LOCK_SUFFIX_LEN ||
195 strcmp(ret.buf + ret.len - LOCK_SUFFIX_LEN, LOCK_SUFFIX))
196 die("BUG: get_locked_file_path() called for malformed lock object");
lockfile.h
+8 -8
@@ -111,7 +111,7 @@
111 #include "tempfile.h"
112
113 struct lock_file {
114 - struct tempfile tempfile;
114 + struct tempfile *tempfile;
115 };
116
117 /* String appended to a filename to derive the lockfile name: */
@@ -180,7 +180,7 @@ static inline int hold_lock_file_for_update(
180 */
181 static inline int is_lock_file_locked(struct lock_file *lk)
182 {
183 - return is_tempfile_active(&lk->tempfile);
183 + return is_tempfile_active(lk->tempfile);
184 }
185
186 /*
@@ -208,7 +208,7 @@ extern NORETURN void unable_to_lock_die(const char *path, int err);
208 */
209 static inline FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)
210 {
211 - return fdopen_tempfile(&lk->tempfile, mode);
211 + return fdopen_tempfile(lk->tempfile, mode);
212 }
213
214 /*
@@ -217,17 +217,17 @@ static inline FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)
217 */
218 static inline const char *get_lock_file_path(struct lock_file *lk)
219 {
220 - return get_tempfile_path(&lk->tempfile);
220 + return get_tempfile_path(lk->tempfile);
221 }
222
223 static inline int get_lock_file_fd(struct lock_file *lk)
224 {
225 - return get_tempfile_fd(&lk->tempfile);
225 + return get_tempfile_fd(lk->tempfile);
226 }
227
228 static inline FILE *get_lock_file_fp(struct lock_file *lk)
229 {
230 - return get_tempfile_fp(&lk->tempfile);
230 + return get_tempfile_fp(lk->tempfile);
231 }
232
233 /*
@@ -246,7 +246,7 @@ extern char *get_locked_file_path(struct lock_file *lk);
246 */
247 static inline int close_lock_file_gently(struct lock_file *lk)
248 {
249 - return close_tempfile_gently(&lk->tempfile);
249 + return close_tempfile_gently(lk->tempfile);
250 }
251
252 /*
@@ -270,7 +270,7 @@ static inline int close_lock_file_gently(struct lock_file *lk)
270 */
271 static inline int reopen_lock_file(struct lock_file *lk)
272 {
273 - return reopen_tempfile(&lk->tempfile);
273 + return reopen_tempfile(lk->tempfile);
274 }
275
276 /*
read-cache.c
+12 -13
@@ -2311,7 +2311,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
2311 return -1;
2312 if (close_tempfile_gently(tempfile)) {
2313 error(_("could not close '%s'"), tempfile->filename.buf);
2314 - delete_tempfile(tempfile);
2314 + delete_tempfile(&tempfile);
2315 return -1;
2316 }
2317 if (stat(tempfile->filename.buf, &st))
@@ -2337,7 +2337,7 @@ static int commit_locked_index(struct lock_file *lk)
2337 static int do_write_locked_index(struct index_state *istate, struct lock_file *lock,
2338 unsigned flags)
2339 {
2340 - int ret = do_write_index(istate, &lock->tempfile, 0);
2340 + int ret = do_write_index(istate, lock->tempfile, 0);
2341 if (ret)
2342 return ret;
2343 assert((flags & (COMMIT_LOCK | CLOSE_LOCK)) !=
@@ -2420,34 +2420,33 @@ static int clean_shared_index_files(const char *current_hex)
2420 return 0;
2421 }
2422
2423 -static struct tempfile temporary_sharedindex;
2424 -
2423 static int write_shared_index(struct index_state *istate,
2424 struct lock_file *lock, unsigned flags)
2425 {
2426 + struct tempfile *temp;
2427 struct split_index *si = istate->split_index;
2429 - int fd, ret;
2428 + int ret;
2429
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) {
2432 hashclr(si->base_sha1);
2433 return do_write_locked_index(istate, lock, flags);
2434 }
2435 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);
2437 if (ret) {
2439 - delete_tempfile(&temporary_sharedindex);
2438 + delete_tempfile(&temp);
2439 return ret;
2440 }
2442 - ret = adjust_shared_perm(get_tempfile_path(&temporary_sharedindex));
2441 + ret = adjust_shared_perm(get_tempfile_path(temp));
2442 if (ret) {
2443 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);
2446 errno = save_errno;
2447 return ret;
2448 }
2450 - ret = rename_tempfile(&temporary_sharedindex,
2449 + ret = rename_tempfile(&temp,
2450 git_path("sharedindex.%s", sha1_to_hex(si->base->sha1)));
2451 if (!ret) {
2452 hashcpy(si->base_sha1, si->base->sha1);
refs/files-backend.c
+2 -2
@@ -1747,12 +1747,12 @@ static int create_symref_locked(struct files_ref_store *refs,
1747
1748 if (!fdopen_lock_file(lock->lk, "w"))
1749 return error("unable to fdopen %s: %s",
1750 - lock->lk->tempfile.filename.buf, strerror(errno));
1750 + lock->lk->tempfile->filename.buf, strerror(errno));
1751
1752 update_symref_reflog(refs, lock, refname, target, logmsg);
1753
1754 /* 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);
1756 if (commit_ref(lock) < 0)
1757 return error("unable to write symref for %s: %s", refname,
1758 strerror(errno));
refs/packed-backend.c
+6 -5
@@ -75,7 +75,7 @@ struct packed_ref_store {
75 * "packed-refs" file. Note that this (and thus the enclosing
76 * `packed_ref_store`) must not be freed.
77 */
78 - struct tempfile tempfile;
78 + struct tempfile *tempfile;
79 };
80
81 struct ref_store *packed_ref_store_create(const char *path,
@@ -628,7 +628,8 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
628 */
629 packed_refs_path = get_locked_file_path(&refs->lock);
630 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) {
633 strbuf_addf(err, "unable to create file %s: %s",
634 sb.buf, strerror(errno));
635 strbuf_release(&sb);
@@ -636,7 +637,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
637 }
638 strbuf_release(&sb);
639
639 - out = fdopen_tempfile(&refs->tempfile, "w");
640 + out = fdopen_tempfile(refs->tempfile, "w");
641 if (!out) {
642 strbuf_addf(err, "unable to fdopen packed-refs tempfile: %s",
643 strerror(errno));
@@ -645,7 +646,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
646
647 if (fprintf(out, "%s", PACKED_REFS_HEADER) < 0) {
648 strbuf_addf(err, "error writing to %s: %s",
648 - get_tempfile_path(&refs->tempfile), strerror(errno));
649 + get_tempfile_path(refs->tempfile), strerror(errno));
650 goto error;
651 }
652
@@ -657,7 +658,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
658 if (write_packed_entry(out, iter->refname, iter->oid->hash,
659 peel_error ? NULL : peeled.hash)) {
660 strbuf_addf(err, "error writing to %s: %s",
660 - get_tempfile_path(&refs->tempfile),
661 + get_tempfile_path(refs->tempfile),
662 strerror(errno));
663 ref_iterator_abort(iter);
664 goto error;
shallow.c
+6 -7
@@ -288,19 +288,18 @@ int write_shallow_commits(struct strbuf *out, int use_pack_protocol,
288
289 const char *setup_temporary_shallow(const struct oid_array *extra)
290 {
291 - static struct tempfile temp;
291 + struct tempfile *temp;
292 struct strbuf sb = STRBUF_INIT;
293 - int fd;
293
294 if (write_shallow_commits(&sb, 0, extra)) {
296 - fd = xmks_tempfile(&temp, git_path("shallow_XXXXXX"));
295 + temp = xmks_tempfile(git_path("shallow_XXXXXX"));
296
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)
299 die_errno("failed to write to %s",
301 - get_tempfile_path(&temp));
300 + get_tempfile_path(temp));
301 strbuf_release(&sb);
303 - return get_tempfile_path(&temp);
302 + return get_tempfile_path(temp);
303 }
304 /*
305 * is_repository_shallow() sees empty string as "no shallow
tempfile.c
+37 -29
@@ -91,14 +91,16 @@ static void remove_tempfiles_on_signal(int signo)
91 raise(signo);
92 }
93
94 -static void prepare_tempfile_object(struct tempfile *tempfile)
94 +static struct tempfile *new_tempfile(void)
95 {
96 + struct tempfile *tempfile = xmalloc(sizeof(*tempfile));
97 tempfile->fd = -1;
98 tempfile->fp = NULL;
99 tempfile->active = 0;
100 tempfile->owner = 0;
101 INIT_LIST_HEAD(&tempfile->list);
102 strbuf_init(&tempfile->filename, 0);
103 + return tempfile;
104 }
105
106 static void activate_tempfile(struct tempfile *tempfile)
@@ -124,12 +126,13 @@ static void deactivate_tempfile(struct tempfile *tempfile)
126 tempfile->active = 0;
127 strbuf_release(&tempfile->filename);
128 volatile_list_del(&tempfile->list);
129 + free(tempfile);
130 }
131
132 /* Make sure errno contains a meaningful value on error */
130 -int create_tempfile(struct tempfile *tempfile, const char *path)
133 +struct tempfile *create_tempfile(const char *path)
134 {
132 - prepare_tempfile_object(tempfile);
135 + struct tempfile *tempfile = new_tempfile();
136
137 strbuf_add_absolute_path(&tempfile->filename, path);
138 tempfile->fd = open(tempfile->filename.buf,
@@ -140,48 +143,47 @@ int create_tempfile(struct tempfile *tempfile, const char *path)
143 O_RDWR | O_CREAT | O_EXCL, 0666);
144 if (tempfile->fd < 0) {
145 deactivate_tempfile(tempfile);
143 - return -1;
146 + return NULL;
147 }
148 activate_tempfile(tempfile);
149 if (adjust_shared_perm(tempfile->filename.buf)) {
150 int save_errno = errno;
151 error("cannot fix permission bits on %s", tempfile->filename.buf);
149 - delete_tempfile(tempfile);
152 + delete_tempfile(&tempfile);
153 errno = save_errno;
151 - return -1;
154 + return NULL;
155 }
153 - return tempfile->fd;
156 +
157 + return tempfile;
158 }
159
156 -void register_tempfile(struct tempfile *tempfile, const char *path)
160 +struct tempfile *register_tempfile(const char *path)
161 {
158 - prepare_tempfile_object(tempfile);
162 + struct tempfile *tempfile = new_tempfile();
163 strbuf_add_absolute_path(&tempfile->filename, path);
164 activate_tempfile(tempfile);
165 + return tempfile;
166 }
167
163 -int mks_tempfile_sm(struct tempfile *tempfile,
164 - const char *template, int suffixlen, int mode)
168 +struct tempfile *mks_tempfile_sm(const char *template, int suffixlen, int mode)
169 {
166 - prepare_tempfile_object(tempfile);
170 + struct tempfile *tempfile = new_tempfile();
171
172 strbuf_add_absolute_path(&tempfile->filename, template);
173 tempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode);
174 if (tempfile->fd < 0) {
175 deactivate_tempfile(tempfile);
172 - return -1;
176 + return NULL;
177 }
178 activate_tempfile(tempfile);
175 - return tempfile->fd;
179 + return tempfile;
180 }
181
178 -int mks_tempfile_tsm(struct tempfile *tempfile,
179 - const char *template, int suffixlen, int mode)
182 +struct tempfile *mks_tempfile_tsm(const char *template, int suffixlen, int mode)
183 {
184 + struct tempfile *tempfile = new_tempfile();
185 const char *tmpdir;
186
183 - prepare_tempfile_object(tempfile);
184 -
187 tmpdir = getenv("TMPDIR");
188 if (!tmpdir)
189 tmpdir = "/tmp";
@@ -190,25 +192,25 @@ int mks_tempfile_tsm(struct tempfile *tempfile,
192 tempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode);
193 if (tempfile->fd < 0) {
194 deactivate_tempfile(tempfile);
193 - return -1;
195 + return NULL;
196 }
197 activate_tempfile(tempfile);
196 - return tempfile->fd;
198 + return tempfile;
199 }
200
199 -int xmks_tempfile_m(struct tempfile *tempfile, const char *template, int mode)
201 +struct tempfile *xmks_tempfile_m(const char *template, int mode)
202 {
201 - int fd;
203 + struct tempfile *tempfile;
204 struct strbuf full_template = STRBUF_INIT;
205
206 strbuf_add_absolute_path(&full_template, template);
205 - fd = mks_tempfile_m(tempfile, full_template.buf, mode);
206 - if (fd < 0)
207 + tempfile = mks_tempfile_m(full_template.buf, mode);
208 + if (!tempfile)
209 die_errno("Unable to create temporary file '%s'",
210 full_template.buf);
211
212 strbuf_release(&full_template);
211 - return fd;
213 + return tempfile;
214 }
215
216 FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode)
@@ -281,33 +283,39 @@ int reopen_tempfile(struct tempfile *tempfile)
283 return tempfile->fd;
284 }
285
284 -int rename_tempfile(struct tempfile *tempfile, const char *path)
286 +int rename_tempfile(struct tempfile **tempfile_p, const char *path)
287 {
288 + struct tempfile *tempfile = *tempfile_p;
289 +
290 if (!is_tempfile_active(tempfile))
291 BUG("rename_tempfile called for inactive object");
292
293 if (close_tempfile_gently(tempfile)) {
290 - delete_tempfile(tempfile);
294 + delete_tempfile(tempfile_p);
295 return -1;
296 }
297
298 if (rename(tempfile->filename.buf, path)) {
299 int save_errno = errno;
296 - delete_tempfile(tempfile);
300 + delete_tempfile(tempfile_p);
301 errno = save_errno;
302 return -1;
303 }
304
305 deactivate_tempfile(tempfile);
306 + *tempfile_p = NULL;
307 return 0;
308 }
309
305 -void delete_tempfile(struct tempfile *tempfile)
310 +void delete_tempfile(struct tempfile **tempfile_p)
311 {
312 + struct tempfile *tempfile = *tempfile_p;
313 +
314 if (!is_tempfile_active(tempfile))
315 return;
316
317 close_tempfile_gently(tempfile);
318 unlink_or_warn(tempfile->filename.buf);
319 deactivate_tempfile(tempfile);
320 + *tempfile_p = NULL;
321 }
tempfile.h
+38 -47
@@ -17,22 +17,18 @@
17 *
18 * The caller:
19 *
20 - * * Allocates a `struct tempfile`. Once the structure is passed to
21 - * `create_tempfile()`, its storage must remain valid until
22 - * `delete_tempfile()` or `rename_tempfile()` is called on it.
23 - *
20 * * Attempts to create a temporary file by calling
25 - * `create_tempfile()`.
21 + * `create_tempfile()`. The resources used for the temporary file are
22 + * managed by the tempfile API.
23 *
24 * * Writes new content to the file by either:
25 *
29 - * * writing to the file descriptor returned by `create_tempfile()`
30 - * (also available via `tempfile->fd`).
26 + * * writing to the `tempfile->fd` file descriptor
27 *
28 * * calling `fdopen_tempfile()` to get a `FILE` pointer for the
29 * open file and writing to the file using stdio.
30 *
35 - * Note that the file descriptor returned by create_tempfile()
31 + * Note that the file descriptor created by create_tempfile()
32 * is marked O_CLOEXEC, so the new contents must be written by
33 * the current process, not any spawned one.
34 *
@@ -50,7 +46,7 @@
46 * `delete_tempfile()` or `rename_tempfile()`.
47 *
48 * After the temporary file is renamed or deleted, the `tempfile`
53 - * object may be reused or freed.
49 + * object is no longer valid and should not be reused.
50 *
51 * If the program exits before `rename_tempfile()` or
52 * `delete_tempfile()` is called, an `atexit(3)` handler will close
@@ -69,8 +65,8 @@
65 * Error handling
66 * --------------
67 *
72 - * `create_tempfile()` returns a file descriptor on success or -1 on
73 - * failure. On errors, `errno` describes the reason for failure.
68 + * `create_tempfile()` returns an allocated tempfile on success or NULL
69 + * on failure. On errors, `errno` describes the reason for failure.
70 *
71 * `delete_tempfile()`, `rename_tempfile()`, and `close_tempfile_gently()`
72 * return 0 on success. On failure they set `errno` appropriately and return
@@ -89,10 +85,10 @@ struct tempfile {
85
86 /*
87 * Attempt to create a temporary file at the specified `path`. Return
92 - * a file descriptor for writing to it, or -1 on error. It is an error
93 - * if a file already exists at that path.
88 + * a tempfile (whose "fd" member can be used for writing to it), or
89 + * NULL on error. It is an error if a file already exists at that path.
90 */
95 -extern int create_tempfile(struct tempfile *tempfile, const char *path);
91 +extern struct tempfile *create_tempfile(const char *path);
92
93 /*
94 * Register an existing file as a tempfile, meaning that it will be
@@ -100,7 +96,7 @@ extern int create_tempfile(struct tempfile *tempfile, const char *path);
96 * but it can be worked with like any other closed tempfile (for
97 * example, it can be opened using reopen_tempfile()).
98 */
103 -extern void register_tempfile(struct tempfile *tempfile, const char *path);
99 +extern struct tempfile *register_tempfile(const char *path);
100
101
102 /*
@@ -132,70 +128,65 @@ extern void register_tempfile(struct tempfile *tempfile, const char *path);
128 * know the (absolute) path of the file that was created, it can be
129 * read from tempfile->filename.
130 *
135 - * On success, the functions return a file descriptor that is open for
136 - * writing the temporary file. On errors, they return -1 and set errno
137 - * appropriately (except for the "x" variants, which die() on errors).
131 + * On success, the functions return a tempfile whose "fd" member is open
132 + * for writing the temporary file. On errors, they return NULL and set
133 + * errno appropriately (except for the "x" variants, which die() on
134 + * errors).
135 */
136
137 /* See "mks_tempfile functions" above. */
141 -extern int mks_tempfile_sm(struct tempfile *tempfile,
142 - const char *template, int suffixlen, int mode);
138 +extern struct tempfile *mks_tempfile_sm(const char *template,
139 + int suffixlen, int mode);
140
141 /* See "mks_tempfile functions" above. */
145 -static inline int mks_tempfile_s(struct tempfile *tempfile,
146 - const char *template, int suffixlen)
142 +static inline struct tempfile *mks_tempfile_s(const char *template,
143 + int suffixlen)
144 {
148 - return mks_tempfile_sm(tempfile, template, suffixlen, 0600);
145 + return mks_tempfile_sm(template, suffixlen, 0600);
146 }
147
148 /* See "mks_tempfile functions" above. */
152 -static inline int mks_tempfile_m(struct tempfile *tempfile,
153 - const char *template, int mode)
149 +static inline struct tempfile *mks_tempfile_m(const char *template, int mode)
150 {
155 - return mks_tempfile_sm(tempfile, template, 0, mode);
151 + return mks_tempfile_sm(template, 0, mode);
152 }
153
154 /* See "mks_tempfile functions" above. */
159 -static inline int mks_tempfile(struct tempfile *tempfile,
160 - const char *template)
155 +static inline struct tempfile *mks_tempfile(const char *template)
156 {
162 - return mks_tempfile_sm(tempfile, template, 0, 0600);
157 + return mks_tempfile_sm(template, 0, 0600);
158 }
159
160 /* See "mks_tempfile functions" above. */
166 -extern int mks_tempfile_tsm(struct tempfile *tempfile,
167 - const char *template, int suffixlen, int mode);
161 +extern struct tempfile *mks_tempfile_tsm(const char *template,
162 + int suffixlen, int mode);
163
164 /* See "mks_tempfile functions" above. */
170 -static inline int mks_tempfile_ts(struct tempfile *tempfile,
171 - const char *template, int suffixlen)
165 +static inline struct tempfile *mks_tempfile_ts(const char *template,
166 + int suffixlen)
167 {
173 - return mks_tempfile_tsm(tempfile, template, suffixlen, 0600);
168 + return mks_tempfile_tsm(template, suffixlen, 0600);
169 }
170
171 /* See "mks_tempfile functions" above. */
177 -static inline int mks_tempfile_tm(struct tempfile *tempfile,
178 - const char *template, int mode)
172 +static inline struct tempfile *mks_tempfile_tm(const char *template, int mode)
173 {
180 - return mks_tempfile_tsm(tempfile, template, 0, mode);
174 + return mks_tempfile_tsm(template, 0, mode);
175 }
176
177 /* See "mks_tempfile functions" above. */
184 -static inline int mks_tempfile_t(struct tempfile *tempfile,
185 - const char *template)
178 +static inline struct tempfile *mks_tempfile_t(const char *template)
179 {
187 - return mks_tempfile_tsm(tempfile, template, 0, 0600);
180 + return mks_tempfile_tsm(template, 0, 0600);
181 }
182
183 /* See "mks_tempfile functions" above. */
191 -extern int xmks_tempfile_m(struct tempfile *tempfile,
192 - const char *template, int mode);
184 +extern struct tempfile *xmks_tempfile_m(const char *template, int mode);
185
186 /* See "mks_tempfile functions" above. */
195 -static inline int xmks_tempfile(struct tempfile *tempfile,
196 - const char *template)
187 +static inline struct tempfile *xmks_tempfile(const char *template)
188 {
198 - return xmks_tempfile_m(tempfile, template, 0600);
189 + return xmks_tempfile_m(template, 0600);
190 }
191
192 /*
@@ -257,7 +248,7 @@ extern int reopen_tempfile(struct tempfile *tempfile);
248 * `delete_tempfile()` for a `tempfile` object that has already been
249 * deleted or renamed.
250 */
260 -extern void delete_tempfile(struct tempfile *tempfile);
251 +extern void delete_tempfile(struct tempfile **tempfile_p);
252
253 /*
254 * Close the file descriptor and/or file pointer if they are still
@@ -268,6 +259,6 @@ extern void delete_tempfile(struct tempfile *tempfile);
259 * `rename(2)`. It is a bug to call `rename_tempfile()` for a
260 * `tempfile` object that is not currently active.
261 */
271 -extern int rename_tempfile(struct tempfile *tempfile, const char *path);
262 +extern int rename_tempfile(struct tempfile **tempfile_p, const char *path);
263
264 #endif /* TEMPFILE_H */
trailer.c
+3 -3
@@ -995,7 +995,7 @@ static void free_all(struct list_head *head)
995 }
996 }
997
998 -static struct tempfile trailers_tempfile;
998 +static struct tempfile *trailers_tempfile;
999
1000 static FILE *create_in_place_tempfile(const char *file)
1001 {
@@ -1017,9 +1017,9 @@ static FILE *create_in_place_tempfile(const char *file)
1017 strbuf_add(&template, file, tail - file + 1);
1018 strbuf_addstr(&template, "git-interpret-trailers-XXXXXX");
1019
1020 - xmks_tempfile_m(&trailers_tempfile, template.buf, st.st_mode);
1020 + trailers_tempfile = xmks_tempfile_m(template.buf, st.st_mode);
1021 strbuf_release(&template);
1022 - outfile = fdopen_tempfile(&trailers_tempfile, "w");
1022 + outfile = fdopen_tempfile(trailers_tempfile, "w");
1023 if (!outfile)
1024 die_errno(_("could not open temporary file"));
1025