Re: [PATCH] config: use a static lock_file struct

From: Jeff King <hidden>
Date: 2017-08-30 04:56:02
Subsystem: the rest · Maintainer: Linus Torvalds

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

quoted

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;
 };

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help