Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> 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 <pclouds@xxxxxxxxx>
> ---
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html