On Wed, Mar 16, 2016 at 06:07:43PM +0800, Hui Yiqun wrote:

+	if (runtime_dir && *runtime_dir)
+		git_runtime_dir = mkpathdup("%s/git/", runtime_dir);
+	else
+		git_runtime_dir = mkpathdup("/tmp/git-%d", uid);

Here we allocate the string, but later we may return NULL on error,
leaking the allocated memory.

+	if (!lstat(git_runtime_dir, &st)) {
+		/*
+		 * As described in XDG base dir spec[1], the subdirectory
+		 * under $XDG_RUNTIME_DIR or its fallback MUST be owned by
+		 * the user, and its unix access mode MUST be 0700.
+		 *
+		 * Calling chmod or chown silently may cause security
+		 * problem if somebody chdir to it, sleep, and then, try
+		 * to open our protected runtime cache or socket.
+		 * So we just put warning and left it to user to solve.
+		 *
+		 * [1]https://specifications.freedesktop.org/basedir-spec/
+		 * basedir-spec-latest.html
+		 */

OK. I think these checks should be sufficient to deal with the /tmp race
I mentioned elsewhere in the thread (assuming that an attacker cannot
flip the uid back and forth in the same way, but that should be true on
Unix systems).

+		if ((st.st_mode & 0777) != S_IRWXU) {
+			fprintf(stderr,
+					"permission of runtime directory '%s' "
+					"MUST be 0700 instead of 0%o\n",
+					git_runtime_dir, (st.st_mode & 0777));
+			return NULL;
+		} else if (st.st_uid != uid) {
+			fprintf(stderr,
+					"owner of runtime directory '%s' "
+					"MUST be %d instead of %d\n",
+					git_runtime_dir, uid, st.st_uid);
+			return NULL;
+		}

Should these be using warning(), rather than a raw fprintf?

+	} else {
+		if (safe_create_leading_directories_const(git_runtime_dir) < 0) {
+			fprintf(stderr,
+					"unable to create directories for '%s'\n",
+					git_runtime_dir);
+			return NULL;
+		}
+		if (mkdir(git_runtime_dir, 0700) < 0) {
+			fprintf(stderr,
+					"unable to mkdir '%s'\n", git_runtime_dir);
+			return NULL;
+		}
+	}

And this retains the un-racy mkdir(). Good.

+	free(git_runtime_dir);
+	return mkpathdup("%s/%s", git_runtime_dir, filename);

This mkpathdup accesses the string we just freed?

It might be easier to just use a strbuf here, and then you can append to
it at the end.

-Peff