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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 2, 2014, 19:51 UTC
Message-ID: <CAPig+cQubZByK3rNPOJy8rfLZpALpW0dOMz55gnFWRPppTAApg@mail.gmail.com>
In-Reply-To: <1393675983-3232-2-git-send-email-pclouds@gmail.com>

On Sat, Mar 1, 2014 at 7:12 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

Show 40 quoted lines

> 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@gmail.com>
> ---
> diff --git a/path.c b/path.c
> index 24594c4..5346700 100644
> --- a/path.c
> +++ b/path.c
> @@ -49,85 +60,70 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)
>         return cleanup_path(buf);
>  }
>
> -static char *vsnpath(char *buf, size_t n, const char *fmt, va_list args)
> +static void vsnpath(struct strbuf *buf, const char *fmt, va_list args)
>  {
>         const char *git_dir = get_git_dir();
> -       size_t len;
> -
> -       len = strlen(git_dir);
> -       if (n < len + 1)
> -               goto bad;
> -       memcpy(buf, git_dir, len);
> -       if (len && !is_dir_sep(git_dir[len-1]))
> -               buf[len++] = '/';
> -       len += vsnprintf(buf + len, n - len, fmt, args);
> -       if (len >= n)
> -               goto bad;
> -       return cleanup_path(buf);
> -bad:
> -       strlcpy(buf, bad_path, n);
> -       return buf;
> +       strbuf_addstr(buf, git_dir);
> +       if (buf->len && !is_dir_sep(buf->buf[buf->len - 1]))
> +               strbuf_addch(buf, '/');
> +       strbuf_vaddf(buf, fmt, args);
> +       strbuf_cleanup_path(buf);
>  }

There's a slight semantic change here. The old code overwrote 'buf', but the new code appends to 'buf'. For someone familiar with sprintf(), or typical va_list or variadic functions there may be an intuitive expectation that 'buf' will be overwritten. Should this code be doing strbuf_reset() as its first action (or should that be the responsibility of the caller who is reusing 'buff')?

Show 16 quoted lines

>  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);
>  }

Prior to this change, it was possible (though probably not recommended) for a caller to append gunk to the returned path up to PATH_MAX without worrying about stomping memory. With the change, this is no longer true. Should the function be changed to return 'const char *' to enforce this restriction?

Show 14 quoted lines

>  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;
>  }

Ditto.

Show 13 quoted lines

>
>  void home_config_paths(char **global, char **xdg, char *file)
> @@ -158,41 +154,27 @@ void home_config_paths(char **global, char **xdg, char *file)
>
>  char *git_path_submodule(const char *path, const char *fmt, ...)
>  {
> -       char *pathname = get_pathname();
> -       struct strbuf buf = STRBUF_INIT;
> +       struct strbuf *buf = get_pathname();
> ...
> +       strbuf_cleanup_path(buf);
> +       return buf->buf;
>  }

And here?

>
>  int validate_headref(const char *path)
> --
> 1.9.0.40.gaa8c3ea