git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Jeff King <peff@peff.net>
To: Michael Haggerty <mhagger@alum.mit.edu>
Cc: Brandon Williams <bmwill@google.com>,
	Lars Schneider <larsxschneider@gmail.com>,
	Martin Ågren <martin.agren@gmail.com>,
	Git Users <git@vger.kernel.org>
Subject: Re: [PATCH] config: use a static lock_file struct
Date: Wed, 30 Aug 2017 00:55:55 -0400
Message-ID: <20170830045555.27xczwo3ql7q4bg3@sigill.intra.peff.net> (raw)
In-Reply-To: <20170830043147.culn63luzdsbpuuw@sigill.intra.peff.net>

On Wed, Aug 30, 2017 at 12:31:47AM -0400, Jeff King wrote:

> > It was surprisingly hard trying to get that code to do the right thing,
> > non-racily, in every error path. Since `remove_tempfiles()` can be
> > called any time (even from a signal handler), the linked list of
> > `tempfile` objects has to be kept valid at all times but can't use
> > mutexes. I didn't have the energy to keep going and make the lock
> > objects freeable.
> > 
> > I suppose the task could be made a bit easier by using `sigprocmask(2)`
> > or `pthread_sigmask(3)` to make its running environment a little bit
> > less hostile.
> 
> I think there are really two levels of carefulness here:
> 
>   1. Avoiding complicated things during a signal handler that may rely
>      on having a sane state from the rest of the program (e.g.,
>      half-formed entries, stdio locks, etc).
> 
>   2. Being truly race-free in the face of a signal arriving while we're
>      running arbitrary code that might have a tempfile struct in a funny
>      state.
> 
> I feel like right now we meet (1) and not (2). But I think if we keep to
> that lower bar of (1), it might not be that bad. We're assuming now that
> there's no race on the tempfile->active flag, for instance. We could
> probably make a similar assumption about putting items onto or taking
> them off of a linked list (it's not really atomic, but a single pointer
> assignment is probably "atomic enough" for our purposes).

Something like this, which AFAICT is about as safe as the existing code
in its list manipulation. It retains the "active" flag as an additional
check which I think isn't strictly necessary, but potentially catches
some logic errors.

The strbuf_reset() calls become strbuf_release(), since we're promising
the caller that they could now free the struct (or let it go out of
scope) if they chose. Probably during a signal handler we should skip
that (we know the struct is off the list and non-active at that point,
but we could possibly hit libc's free() mutex).

diff --git a/tempfile.c b/tempfile.c
index 6843710670..a7d964ebf8 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -69,7 +69,6 @@ static void remove_tempfiles(int skip_fclose)
 				tempfile_list->fp = NULL;
 			delete_tempfile(tempfile_list);
 		}
-		tempfile_list = tempfile_list->next;
 	}
 }
 
@@ -85,39 +84,54 @@ static void remove_tempfiles_on_signal(int signo)
 	raise(signo);
 }
 
-/*
- * Initialize *tempfile if necessary and add it to tempfile_list.
- */
-static void prepare_tempfile_object(struct tempfile *tempfile)
+static void init_tempfile(struct tempfile *tempfile)
+{
+	tempfile->fd = -1;
+	tempfile->fp = NULL;
+	tempfile->active = 0;
+	tempfile->owner = 0;
+	tempfile->next = NULL;
+	strbuf_init(&tempfile->filename, 0);
+}
+
+static void activate_tempfile(struct tempfile *tempfile)
 {
-	if (!tempfile_list) {
-		/* One-time initialization */
+	static volatile int initialized;
+
+	if (!initialized) {
 		sigchain_push_common(remove_tempfiles_on_signal);
 		atexit(remove_tempfiles_on_exit);
+		initialized = 1;
 	}
 
 	if (tempfile->active)
-		die("BUG: prepare_tempfile_object called for active object");
-	if (!tempfile->on_list) {
-		/* Initialize *tempfile and add it to tempfile_list: */
-		tempfile->fd = -1;
-		tempfile->fp = NULL;
-		tempfile->active = 0;
-		tempfile->owner = 0;
-		strbuf_init(&tempfile->filename, 0);
-		tempfile->next = tempfile_list;
-		tempfile_list = tempfile;
-		tempfile->on_list = 1;
-	} else if (tempfile->filename.len) {
-		/* This shouldn't happen, but better safe than sorry. */
-		die("BUG: prepare_tempfile_object called for improperly-reset object");
+		die("BUG: activate_tempfile called for active object");
+
+	tempfile->next = tempfile_list;
+	tempfile_list = tempfile;
+	tempfile->active = 1;
+}
+
+static void deactivate_tempfile(struct tempfile *tempfile)
+{
+	struct tempfile *volatile *p;
+
+	if (!tempfile->active)
+		return;
+
+	tempfile->active = 0;
+	for (p = &tempfile_list; *p; p = &(*p)->next) {
+		if (*p == tempfile) {
+			*p = tempfile->next;
+			break;
+		}
 	}
 }
 
 /* Make sure errno contains a meaningful value on error */
 int create_tempfile(struct tempfile *tempfile, const char *path)
 {
-	prepare_tempfile_object(tempfile);
+	init_tempfile(tempfile);
 
 	strbuf_add_absolute_path(&tempfile->filename, path);
 	tempfile->fd = open(tempfile->filename.buf,
@@ -127,11 +141,11 @@ int create_tempfile(struct tempfile *tempfile, const char *path)
 		tempfile->fd = open(tempfile->filename.buf,
 				    O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (tempfile->fd < 0) {
-		strbuf_reset(&tempfile->filename);
+		strbuf_release(&tempfile->filename);
 		return -1;
 	}
 	tempfile->owner = getpid();
-	tempfile->active = 1;
+	activate_tempfile(tempfile);
 	if (adjust_shared_perm(tempfile->filename.buf)) {
 		int save_errno = errno;
 		error("cannot fix permission bits on %s", tempfile->filename.buf);
@@ -144,25 +158,25 @@ int create_tempfile(struct tempfile *tempfile, const char *path)
 
 void register_tempfile(struct tempfile *tempfile, const char *path)
 {
-	prepare_tempfile_object(tempfile);
+	init_tempfile(tempfile);
 	strbuf_add_absolute_path(&tempfile->filename, path);
 	tempfile->owner = getpid();
-	tempfile->active = 1;
+	activate_tempfile(tempfile);
 }
 
 int mks_tempfile_sm(struct tempfile *tempfile,
 		    const char *template, int suffixlen, int mode)
 {
-	prepare_tempfile_object(tempfile);
+	init_tempfile(tempfile);
 
 	strbuf_add_absolute_path(&tempfile->filename, template);
 	tempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode);
 	if (tempfile->fd < 0) {
-		strbuf_reset(&tempfile->filename);
+		strbuf_release(&tempfile->filename);
 		return -1;
 	}
 	tempfile->owner = getpid();
-	tempfile->active = 1;
+	activate_tempfile(tempfile);
 	return tempfile->fd;
 }
 
@@ -171,7 +185,7 @@ int mks_tempfile_tsm(struct tempfile *tempfile,
 {
 	const char *tmpdir;
 
-	prepare_tempfile_object(tempfile);
+	init_tempfile(tempfile);
 
 	tmpdir = getenv("TMPDIR");
 	if (!tmpdir)
@@ -180,11 +194,11 @@ int mks_tempfile_tsm(struct tempfile *tempfile,
 	strbuf_addf(&tempfile->filename, "%s/%s", tmpdir, template);
 	tempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode);
 	if (tempfile->fd < 0) {
-		strbuf_reset(&tempfile->filename);
+		strbuf_release(&tempfile->filename);
 		return -1;
 	}
 	tempfile->owner = getpid();
-	tempfile->active = 1;
+	activate_tempfile(tempfile);
 	return tempfile->fd;
 }
 
@@ -293,8 +307,8 @@ int rename_tempfile(struct tempfile *tempfile, const char *path)
 		return -1;
 	}
 
-	tempfile->active = 0;
-	strbuf_reset(&tempfile->filename);
+	deactivate_tempfile(tempfile);
+	strbuf_release(&tempfile->filename);
 	return 0;
 }
 
@@ -305,7 +319,7 @@ void delete_tempfile(struct tempfile *tempfile)
 
 	if (!close_tempfile(tempfile)) {
 		unlink_or_warn(tempfile->filename.buf);
-		tempfile->active = 0;
-		strbuf_reset(&tempfile->filename);
+		deactivate_tempfile(tempfile);
+		strbuf_release(&tempfile->filename);
 	}
 }
diff --git a/tempfile.h b/tempfile.h
index 2f0038decd..df96f82e84 100644
--- a/tempfile.h
+++ b/tempfile.h
@@ -85,7 +85,6 @@ struct tempfile {
 	volatile int fd;
 	FILE *volatile fp;
 	volatile pid_t owner;
-	char on_list;
 	struct strbuf filename;
 };
 

  parent reply index

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-27  7:37 [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1() Martin Ågren
2017-08-27 15:41 ` Jeff King
2017-08-27 18:25   ` Jeff King
2017-08-27 18:21 ` Lars Schneider
2017-08-27 19:09   ` Martin Ågren
2017-08-27 19:15     ` Jeff King
2017-08-27 20:04     ` Lars Schneider
2017-08-27 23:23       ` Jeff King
2017-08-28  4:11         ` Martin Ågren
2017-08-28 17:52           ` Stefan Beller
2017-08-28 17:58             ` Jeff King
2017-09-05 10:03               ` Junio C Hamano
2017-08-29 17:51           ` Lars Schneider
2017-08-29 18:53             ` Jeff King
2017-08-29 18:58               ` [PATCH] config: use a static lock_file struct Jeff King
2017-08-29 19:09                 ` Brandon Williams
2017-08-29 19:10                   ` Brandon Williams
2017-08-29 19:12                   ` Jeff King
2017-08-30  3:25                     ` Michael Haggerty
2017-08-30  4:31                       ` Jeff King
2017-08-30  4:55                         ` Michael Haggerty
2017-08-30  4:55                         ` Jeff King [this message]
2017-08-30  5:55                           ` Jeff King
2017-08-30  7:07                             ` Michael Haggerty
2017-09-02  8:44                               ` Jeff King
2017-09-02 13:50                                 ` Jeff King
2017-08-30  6:55                           ` Michael Haggerty
2017-08-30 19:53                             ` Jeff King
2017-08-30 19:57                               ` Brandon Williams
2017-08-30 20:11                                 ` Jeff King
2017-08-30 21:06                                   ` Brandon Williams
2017-08-31  4:09                                     ` Jeff King
2017-09-06  3:59                 ` Junio C Hamano
2017-09-06 12:41                   ` Jeff King
2017-08-29 19:22               ` [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1() Martin Ågren
2017-08-29 21:48                 ` Jeff King
2017-08-30  5:31                   ` Jeff King
2017-09-05 10:03                     ` Junio C Hamano
2017-10-10  4:06                       ` [PATCH 0/2] Do not call cmd_*() as a subroutine Junio C Hamano
2017-10-10  4:06                         ` [PATCH 1/2] describe: do not use " Junio C Hamano
2017-10-10  4:06                         ` [PATCH 2/2] merge-ours: " Junio C Hamano

Reply instructions:

You may reply publically 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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=20170830045555.27xczwo3ql7q4bg3@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=larsxschneider@gmail.com \
    --cc=martin.agren@gmail.com \
    --cc=mhagger@alum.mit.edu \
    /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

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox