Re: [PATCH 02/12] environment: move strbuf into block to plug leak

From

René Scharfe <l.s.r@web.de>

Date

Jun 26, 2021, 08:27 UTC

Message-ID

<15df4c07-988e-baff-8760-a199ec9aeb1b@web.de>

In-Reply-To

<CABPp-BGFH787gsw-yd3BpLt_rDe2zDoSFP6mMx6PSQfy1Ct4vw@mail.gmail.com>

Am 21.06.21 um 22:49 schrieb Elijah Newren:

> On Sun, Jun 20, 2021 at 8:14 AM <andrzej@ahunt.org> wrote:
>> It looks like this leak has existed since realpath was first added to
>> set_git_work_tree() in:
>>   3d7747e318 (real_path: remove unsafe API, 2020-03-10)
>
> Looking at that commit, it appears to have introduced other problems.
> For example, the documentation for read_gitfile_gently() claims it
> returns a value from a shared buffer, but that commit got rid of the
> shared buffer so the documentation is no longer accurate.  The thing
> that is returned is either the path that was passed in, or some newly
> allocated path that differs, in which case the caller would be
> responsible to free() it, but it looks like the callers aren't doing
> so.

diff --git a/setup.c b/setup.c
index ead2f80cd8..75b0a4bea6 100644
--- a/setup.c
+++ b/setup.c
@@ -720,86 +720,87 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
 /*
  * Try to read the location of the git directory from the .git file,
  * return path to git directory if found. The return value comes from
  * a shared buffer.
  *
  * On failure, if return_error_code is not NULL, return_error_code
  * will be set to an error code and NULL will be returned. If
  * return_error_code is NULL the function will die instead (for most
  * cases).
  */
 const char *read_gitfile_gently(const char *path, int *return_error_code)
 {
 	const int max_file_size = 1 << 20;  /* 1MB */
 	int error_code = 0;
 	char *buf = NULL;
 	char *dir = NULL;
 	const char *slash;
 	struct stat st;
 	int fd;
 	ssize_t len;
 	static struct strbuf realpath = STRBUF_INIT;
+	const char *result = NULL;

 	if (stat(path, &st)) {
 		/* NEEDSWORK: discern between ENOENT vs other errors */
 		error_code = READ_GITFILE_ERR_STAT_FAILED;
 		goto cleanup_return;
 	}
 	if (!S_ISREG(st.st_mode)) {
 		error_code = READ_GITFILE_ERR_NOT_A_FILE;
 		goto cleanup_return;
 	}
 	if (st.st_size > max_file_size) {
 		error_code = READ_GITFILE_ERR_TOO_LARGE;
 		goto cleanup_return;
 	}
 	fd = open(path, O_RDONLY);
 	if (fd < 0) {
 		error_code = READ_GITFILE_ERR_OPEN_FAILED;
 		goto cleanup_return;
 	}
 	buf = xmallocz(st.st_size);
 	len = read_in_full(fd, buf, st.st_size);
 	close(fd);
 	if (len != st.st_size) {
 		error_code = READ_GITFILE_ERR_READ_FAILED;
 		goto cleanup_return;
 	}
 	if (!starts_with(buf, "gitdir: ")) {
 		error_code = READ_GITFILE_ERR_INVALID_FORMAT;
 		goto cleanup_return;
 	}
 	while (buf[len - 1] == '\n' || buf[len - 1] == '\r')
 		len--;
 	if (len < 9) {
 		error_code = READ_GITFILE_ERR_NO_PATH;
 		goto cleanup_return;
 	}
 	buf[len] = '\0';
 	dir = buf + 8;

 	if (!is_absolute_path(dir) && (slash = strrchr(path, '/'))) {
 		size_t pathlen = slash+1 - path;
 		dir = xstrfmt("%.*s%.*s", (int)pathlen, path,
 			      (int)(len - 8), buf + 8);
 		free(buf);
 		buf = dir;
 	}
 	if (!is_git_directory(dir)) {
 		error_code = READ_GITFILE_ERR_NOT_A_REPO;
 		goto cleanup_return;
 	}

 	strbuf_realpath(&realpath, dir, 1);
-	path = realpath.buf;
+	result = realpath.buf;

 cleanup_return:
 	if (return_error_code)
 		*return_error_code = error_code;
 	else if (error_code)
 		read_gitfile_error_die(error_code, path, dir);

 	free(buf);
-	return error_code ? NULL : path;
+	return result;
 }

 static const char *setup_explicit_git_dir(const char *gitdirenv,