All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Subject: [PATCH 05/20] always check return value of close_tempfile
Date: Tue, 5 Sep 2017 08:14:26 -0400	[thread overview]
Message-ID: <20170905121426.sc4u4unmwzhamjsa@sigill.intra.peff.net> (raw)
In-Reply-To: <20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net>

If close_tempfile() encounters an error, then it deletes the
tempfile and resets the "struct tempfile". But many code
paths ignore the return value and continue to use the
tempfile. Instead, we should generally treat this the same
as a write() error.

Note that in the postimage of some of these cases our error
message will be bogus after a failed close because we look
at tempfile->filename (either directly or via get_tempfile_path).
But after the failed close resets the tempfile object, this
is guaranteed to be the empty string. That will be addressed
in a future patch (because there are many more cases of the
same problem than just these instances).

Note also in the hunk in gpg-interface.c that it's fine to
call delete_tempfile() in the error path, even if
close_tempfile() failed and already deleted the file. The
tempfile code is smart enough to know the second deletion is
a noop.

Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c          | 4 ++--
 gpg-interface.c | 4 ++--
 shallow.c       | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/diff.c b/diff.c
index 3d3e553a98..fdb974014b 100644
--- a/diff.c
+++ b/diff.c
@@ -3738,9 +3738,9 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,
 		blob = buf.buf;
 		size = buf.len;
 	}
-	if (write_in_full(fd, blob, size) != size)
+	if (write_in_full(fd, blob, size) != size ||
+	    close_tempfile(&temp->tempfile))
 		die_errno("unable to write temp-file");
-	close_tempfile(&temp->tempfile);
 	temp->name = get_tempfile_path(&temp->tempfile);
 	oid_to_hex_r(temp->hex, oid);
 	xsnprintf(temp->mode, sizeof(temp->mode), "%06o", mode);
diff --git a/gpg-interface.c b/gpg-interface.c
index 455b6c04b4..05ca6ecbfd 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -209,13 +209,13 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 	fd = mks_tempfile_t(&temp, ".git_vtag_tmpXXXXXX");
 	if (fd < 0)
 		return error_errno(_("could not create temporary file"));
-	if (write_in_full(fd, signature, signature_size) < 0) {
+	if (write_in_full(fd, signature, signature_size) < 0 ||
+	    close_tempfile(&temp) < 0) {
 		error_errno(_("failed writing detached signature to '%s'"),
 			    temp.filename.buf);
 		delete_tempfile(&temp);
 		return -1;
 	}
-	close_tempfile(&temp);
 
 	argv_array_pushl(&gpg.args,
 			 gpg_program,
diff --git a/shallow.c b/shallow.c
index c7fd68ace0..f49e6ae122 100644
--- a/shallow.c
+++ b/shallow.c
@@ -295,10 +295,10 @@ const char *setup_temporary_shallow(const struct oid_array *extra)
 	if (write_shallow_commits(&sb, 0, extra)) {
 		fd = xmks_tempfile(&temp, git_path("shallow_XXXXXX"));
 
-		if (write_in_full(fd, sb.buf, sb.len) != sb.len)
+		if (write_in_full(fd, sb.buf, sb.len) != sb.len ||
+		    close_tempfile(&temp) < 0)
 			die_errno("failed to write to %s",
 				  get_tempfile_path(&temp));
-		close_tempfile(&temp);
 		strbuf_release(&sb);
 		return get_tempfile_path(&temp);
 	}
-- 
2.14.1.721.gc5bc1565f1


  parent reply	other threads:[~2017-09-05 12:14 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-05 12:13 [PATCH 0/20] allowing struct lock_file to be deleted Jeff King
2017-09-05 12:14 ` [PATCH 01/20] write_index_as_tree: cleanup tempfile on error Jeff King
2017-09-05 12:14 ` [PATCH 02/20] setup_temporary_shallow: avoid using inactive tempfile Jeff King
2017-09-05 12:14 ` [PATCH 03/20] setup_temporary_shallow: move tempfile struct into function Jeff King
2017-09-05 12:14 ` [PATCH 04/20] verify_signed_buffer: prefer close_tempfile() to close() Jeff King
2017-09-05 12:14 ` Jeff King [this message]
2017-09-05 12:14 ` [PATCH 06/20] tempfile: do not delete tempfile on failed close Jeff King
2017-09-05 12:14 ` [PATCH 07/20] lockfile: do not rollback lock " Jeff King
2017-09-05 12:14 ` [PATCH 08/20] tempfile: prefer is_tempfile_active to bare access Jeff King
2017-09-05 12:14 ` [PATCH 09/20] tempfile: handle NULL tempfile pointers gracefully Jeff King
2017-09-05 12:14 ` [PATCH 10/20] tempfile: replace die("BUG") with BUG() Jeff King
2017-09-05 12:14 ` [PATCH 11/20] tempfile: factor out activation Jeff King
2017-09-05 12:14 ` [PATCH 12/20] tempfile: factor out deactivation Jeff King
2017-09-05 12:14 ` [PATCH 13/20] tempfile: robustify cleanup handler Jeff King
2017-09-05 12:14 ` [PATCH 14/20] tempfile: release deactivated strbufs instead of resetting Jeff King
2017-09-05 12:15 ` [PATCH 15/20] tempfile: use list.h for linked list Jeff King
2017-09-05 12:15 ` [PATCH 16/20] tempfile: remove deactivated list entries Jeff King
2017-09-05 12:15 ` [PATCH 17/20] tempfile: auto-allocate tempfiles on heap Jeff King
2017-09-05 12:15 ` [PATCH 18/20] lockfile: update lifetime requirements in documentation Jeff King
2017-09-05 12:15 ` [PATCH 19/20] ref_lock: stop leaking lock_files Jeff King
2017-09-05 12:15 ` [PATCH 20/20] stop leaking lock structs in some simple cases Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170905121426.sc4u4unmwzhamjsa@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.