On Mon, Mar 3, 2014 at 2:51 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Sat, Mar 1, 2014 at 7:12 AM, Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> wrote:
>> 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>
>> ---
>> 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')?
those callers that use get_pathname() already have strbuf reset, so
I'd say let the remaining callers reset the buffer.
>
>> 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?
Sure. Will do (same for below)
--
Duy
--
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