Commit 1fafead53a57d8951e21907cd6bc0d9b8c7a6ffd

Edward Thomson 2016-07-29T12:59:42

sysdir: use the standard `init` pattern Don't try to determine when sysdirs are uninitialized. Instead, simply initialize them all at `git_libgit2_init` time and never try to reinitialize, except when consumers explicitly call `git_sysdir_set`. Looking at the buffer length is especially problematic, since there may no appropriate path for that value. (For example, the Windows-specific programdata directory has no value on non-Windows machines.) Previously we would continually trying to re-lookup these values, which could get racy if two different threads are each calling `git_sysdir_get` and trying to lookup / clear the value simultaneously.

diff --git a/src/sysdir.c b/src/sysdir.c
index bf53d83..29e53e2 100644
--- a/src/sysdir.c
+++ b/src/sysdir.c
@@ -83,45 +83,43 @@ static int git_sysdir_guess_template_dirs(git_buf *out)
 #endif
 }
 
-typedef int (*git_sysdir_guess_cb)(git_buf *out);
-
-static git_buf git_sysdir__dirs[GIT_SYSDIR__MAX] =
-	{ GIT_BUF_INIT, GIT_BUF_INIT, GIT_BUF_INIT, GIT_BUF_INIT, GIT_BUF_INIT };
+struct git_sysdir__dir {
+	git_buf buf;
+	int (*guess)(git_buf *out);
+};
 
-static git_sysdir_guess_cb git_sysdir__dir_guess[GIT_SYSDIR__MAX] = {
-	git_sysdir_guess_system_dirs,
-	git_sysdir_guess_global_dirs,
-	git_sysdir_guess_xdg_dirs,
-	git_sysdir_guess_programdata_dirs,
-	git_sysdir_guess_template_dirs,
+static struct git_sysdir__dir git_sysdir__dirs[] = {
+	{ GIT_BUF_INIT, git_sysdir_guess_system_dirs },
+	{ GIT_BUF_INIT, git_sysdir_guess_global_dirs },
+	{ GIT_BUF_INIT, git_sysdir_guess_xdg_dirs },
+	{ GIT_BUF_INIT, git_sysdir_guess_programdata_dirs },
+	{ GIT_BUF_INIT, git_sysdir_guess_template_dirs },
 };
 
-static int git_sysdir__dirs_shutdown_set = 0;
+static void git_sysdir_global_shutdown(void)
+{
+	size_t i;
+
+	for (i = 0; i < ARRAY_SIZE(git_sysdir__dirs); ++i)
+		git_buf_free(&git_sysdir__dirs[i].buf);
+}
 
 int git_sysdir_global_init(void)
 {
-	git_sysdir_t i;
-	const git_buf *path;
+	size_t i;
 	int error = 0;
 
-	for (i = 0; !error && i < GIT_SYSDIR__MAX; i++)
-		error = git_sysdir_get(&path, i);
+	for (i = 0; !error && i < ARRAY_SIZE(git_sysdir__dirs); i++)
+		error = git_sysdir__dirs[i].guess(&git_sysdir__dirs[i].buf);
 
-	return error;
-}
+	git__on_shutdown(git_sysdir_global_shutdown);
 
-void git_sysdir_global_shutdown(void)
-{
-	int i;
-	for (i = 0; i < GIT_SYSDIR__MAX; ++i)
-		git_buf_free(&git_sysdir__dirs[i]);
-
-	git_sysdir__dirs_shutdown_set = 0;
+	return error;
 }
 
 static int git_sysdir_check_selector(git_sysdir_t which)
 {
-	if (which < GIT_SYSDIR__MAX)
+	if (which < ARRAY_SIZE(git_sysdir__dirs))
 		return 0;
 
 	giterr_set(GITERR_INVALID, "config directory selector out of range");
@@ -137,18 +135,7 @@ int git_sysdir_get(const git_buf **out, git_sysdir_t which)
 
 	GITERR_CHECK_ERROR(git_sysdir_check_selector(which));
 
-	if (!git_buf_len(&git_sysdir__dirs[which])) {
-		/* prepare shutdown if we're going to need it */
-		if (!git_sysdir__dirs_shutdown_set) {
-			git__on_shutdown(git_sysdir_global_shutdown);
-			git_sysdir__dirs_shutdown_set = 1;
-		}
-
-		GITERR_CHECK_ERROR(
-			git_sysdir__dir_guess[which](&git_sysdir__dirs[which]));
-	}
-
-	*out = &git_sysdir__dirs[which];
+	*out = &git_sysdir__dirs[which].buf;
 	return 0;
 }
 
@@ -183,31 +170,38 @@ int git_sysdir_set(git_sysdir_t which, const char *search_path)
 	if (search_path != NULL)
 		expand_path = strstr(search_path, PATH_MAGIC);
 
-	/* init with default if not yet done and needed (ignoring error) */
-	if ((!search_path || expand_path) &&
-		!git_buf_len(&git_sysdir__dirs[which]))
-		git_sysdir__dir_guess[which](&git_sysdir__dirs[which]);
+	/* reset the default if this path has been cleared */
+	if (!search_path || expand_path)
+		git_sysdir__dirs[which].guess(&git_sysdir__dirs[which].buf);
 
 	/* if $PATH is not referenced, then just set the path */
-	if (!expand_path)
-		return git_buf_sets(&git_sysdir__dirs[which], search_path);
+	if (!expand_path) {
+		if (search_path)
+			git_buf_sets(&git_sysdir__dirs[which].buf, search_path);
+
+		goto done;
+	}
 
 	/* otherwise set to join(before $PATH, old value, after $PATH) */
 	if (expand_path > search_path)
 		git_buf_set(&merge, search_path, expand_path - search_path);
 
-	if (git_buf_len(&git_sysdir__dirs[which]))
+	if (git_buf_len(&git_sysdir__dirs[which].buf))
 		git_buf_join(&merge, GIT_PATH_LIST_SEPARATOR,
-			merge.ptr, git_sysdir__dirs[which].ptr);
+			merge.ptr, git_sysdir__dirs[which].buf.ptr);
 
 	expand_path += strlen(PATH_MAGIC);
 	if (*expand_path)
 		git_buf_join(&merge, GIT_PATH_LIST_SEPARATOR, merge.ptr, expand_path);
 
-	git_buf_swap(&git_sysdir__dirs[which], &merge);
+	git_buf_swap(&git_sysdir__dirs[which].buf, &merge);
 	git_buf_free(&merge);
 
-	return git_buf_oom(&git_sysdir__dirs[which]) ? -1 : 0;
+done:
+	if (git_buf_oom(&git_sysdir__dirs[which].buf))
+		return -1;
+
+	return 0;
 }
 
 static int git_sysdir_find_in_dirlist(
diff --git a/src/sysdir.h b/src/sysdir.h
index 12874fc..1187898 100644
--- a/src/sysdir.h
+++ b/src/sysdir.h
@@ -103,9 +103,4 @@ extern int git_sysdir_get_str(char *out, size_t outlen, git_sysdir_t which);
  */
 extern int git_sysdir_set(git_sysdir_t which, const char *paths);
 
-/**
- * Free the configuration file search paths.
- */
-extern void git_sysdir_global_shutdown(void);
-
 #endif /* INCLUDE_sysdir_h__ */