Re: [PATCH v3 01/25] path.c: make get_pathname() return strbuf instead of static buffer

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

We've been avoiding PATH_MAX whenever possible. This patch makes
get_pathname() return a strbuf and updates the callers to take
advantage of this. The code is simplified as we no longer need to
worry about buffer overflow.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 path.c | 119 +++++++++++++++++++++++++++--------------------------------------
 1 file changed, 50 insertions(+), 69 deletions(-)

Nice.

 char *git_pathdup(const char *fmt, ...)
 {
-	char path[PATH_MAX], *ret;
+	struct strbuf *path = get_pathname();
 	va_list args;
 	va_start(args, fmt);
-	ret = vsnpath(path, sizeof(path), fmt, args);
+	vsnpath(path, fmt, args);
 	va_end(args);
-	return xstrdup(ret);
+	return strbuf_detach(path, NULL);
 }

This feels somewhat wrong.

This function is for callers who are willing to take ownership of
the path buffer and promise to free the returned buffer when they
are done, because you are returning strbuf_detach()'ed piece of
memory, giving the ownership away.

The whole point of using get_pathname() is to allow callers not to
care about allocation issues on the paths they scribble on during
their short-and-simple codepaths that do not have too many uses of
similar temporary path buffers.  Why borrow from that round-robin
pool (which may now cause some codepaths to overflow the number of
such active temporary path buffers---have they been all audited)?

Is there a reason not to do just an equivalent of

    #define git_pathdup mkpathdup

and be done with it?  Am I missing something?

 char *mkpathdup(const char *fmt, ...)
 {
-	char *path;
 	struct strbuf sb = STRBUF_INIT;
 	va_list args;
-
 	va_start(args, fmt);
 	strbuf_vaddf(&sb, fmt, args);
 	va_end(args);
-	path = xstrdup(cleanup_path(sb.buf));
-
-	strbuf_release(&sb);
-	return path;
+	strbuf_cleanup_path(&sb);
+	return strbuf_detach(&sb, NULL);
 }
 
 char *mkpath(const char *fmt, ...)
 {
 	va_list args;
-	unsigned len;
-	char *pathname = get_pathname();
-
+	struct strbuf *pathname = get_pathname();
 	va_start(args, fmt);
-	len = vsnprintf(pathname, PATH_MAX, fmt, args);
+	strbuf_vaddf(pathname, fmt, args);
 	va_end(args);
-	if (len >= PATH_MAX)
-		return bad_path;
-	return cleanup_path(pathname);
+	return cleanup_path(pathname->buf);
 }

On the other hand, this one does seem correct.

 char *git_path(const char *fmt, ...)
 {
-	char *pathname = get_pathname();
+	struct strbuf *pathname = get_pathname();
 	va_list args;
-	char *ret;
-
 	va_start(args, fmt);
-	ret = vsnpath(pathname, PATH_MAX, fmt, args);
+	vsnpath(pathname, fmt, args);
 	va_end(args);
-	return ret;
+	return pathname->buf;
 }

So does this.

Thanks.

On Thu, Feb 20, 2014 at 6:26 AM, Junio C Hamano [off-list ref] wrote:

Is there a reason not to do just an equivalent of

    #define git_pathdup mkpathdup

and be done with it?  Am I missing something?

They have a subtle difference: mkpathdup() calls cleanup_path() while
git_pathdup() does not. Without auditing all call sites, we can't be
sure this difference is insignificant. So I keep both functions
separate for now.
-- 
Duy