Re: [PATCH 1/2] entry.c: convert checkout_entry to use strbuf
On Wed, Oct 23, 2013 at 11:09:27AM -0700, Junio C Hamano wrote: > > This is not something you introduced, but while we are here, you may > > want to use ce->namelen, which would be a little faster than treating it > > as a string (especially for strbuf, as it can then know up front how big > > the size is). > > H, do you mean something like this on top? > > diff --git a/entry.c b/entry.c > index d955af5..0d48292 100644 > --- a/entry.c > +++ b/entry.c > @@ -246,7 +246,9 @@ int checkout_entry(struct cache_entry *ce, > return write_entry(ce, topath, state, 1); > > strbuf_reset(&path_buf); > - strbuf_addf(&path_buf, "%.*s%s", state->base_dir_len, state->base_dir, > ce->name); > + strbuf_addf(&path_buf, "%.*s%.*s", > + state->base_dir_len, state->base_dir, > + ce_namelen(ce), ce->name); > path = path_buf.buf; > len = path_buf.len; Yes, though I actually find Erik's version with two separate strbuf_add invocations slightly more readable (it _could_ result in two allocations, but again, we are amortizing the growth over many calls anyway, so most of them will not need to grow the buffer at all). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] entry.c: convert checkout_entry to use strbuf
Jeff King writes: > On Wed, Oct 23, 2013 at 07:55:06PM +0700, Nguyen Thai Ngoc Duy wrote: > ... >> -memcpy(path, state->base_dir, len); >> -strcpy(path + len, ce->name); >> -len += ce_namelen(ce); >> +strbuf_reset(&path_buf); >> +strbuf_addf(&path_buf, "%.*s%s", state->base_dir_len, state->base_dir, >> ce->name); >> +path = path_buf.buf; >> +len = path_buf.len; > > This is not something you introduced, but while we are here, you may > want to use ce->namelen, which would be a little faster than treating it > as a string (especially for strbuf, as it can then know up front how big > the size is). H, do you mean something like this on top? diff --git a/entry.c b/entry.c index d955af5..0d48292 100644 --- a/entry.c +++ b/entry.c @@ -246,7 +246,9 @@ int checkout_entry(struct cache_entry *ce, return write_entry(ce, topath, state, 1); strbuf_reset(&path_buf); - strbuf_addf(&path_buf, "%.*s%s", state->base_dir_len, state->base_dir, ce->name); + strbuf_addf(&path_buf, "%.*s%.*s", + state->base_dir_len, state->base_dir, + ce_namelen(ce), ce->name); path = path_buf.buf; len = path_buf.len; -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] entry.c: convert checkout_entry to use strbuf
On Wed, Oct 23, 2013 at 07:34:18PM +0200, Erik Faye-Lund wrote: > >> - memcpy(path, state->base_dir, len); > >> - strcpy(path + len, ce->name); > >> - len += ce_namelen(ce); > >> + strbuf_reset(&path_buf); > >> + strbuf_addf(&path_buf, "%.*s%s", state->base_dir_len, > >> state->base_dir, ce->name); > >> + path = path_buf.buf; > >> + len = path_buf.len; > > > > This is not something you introduced, but while we are here, you may > > want to use ce->namelen, which would be a little faster than treating it > > as a string (especially for strbuf, as it can then know up front how big > > the size is). > > > > I doubt it's measurable, though (especially as the growth cost is > > amortized due to the static buffer). > > I somehow feel that: > > strbuf_reset(&path_buf); > strbuf_add(&path_buf, state->base_dir, state->base_dir_len); > strbuf_addch(&path_buf, '/'); > strbuf_add(&path_buf, state->name, state->name_len); > > feels a bit neater than using strbuf_addf. But that might just be me. I agree. But note that your addch is a bug. :) -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] entry.c: convert checkout_entry to use strbuf
On Wed, Oct 23, 2013 at 7:29 PM, Jeff King wrote: > On Wed, Oct 23, 2013 at 07:55:06PM +0700, Nguyen Thai Ngoc Duy wrote: > >> The old code does not do boundary check so any paths longer than >> PATH_MAX can cause buffer overflow. Replace it with strbuf to handle >> paths of arbitrary length. > > I think this is a reasonable solution. If we have such a long path, we > are probably about to feed it to open() or another syscall, and we will > just get ENAMETOOLONG there anyway. But certainly we need to fix the > buffer overflow, and we are probably better off letting the syscall > report failure than calling die(), because we generally handle the > syscall failure more gracefully (e.g., by reporting the failed path but > continuing). > >> - memcpy(path, state->base_dir, len); >> - strcpy(path + len, ce->name); >> - len += ce_namelen(ce); >> + strbuf_reset(&path_buf); >> + strbuf_addf(&path_buf, "%.*s%s", state->base_dir_len, state->base_dir, >> ce->name); >> + path = path_buf.buf; >> + len = path_buf.len; > > This is not something you introduced, but while we are here, you may > want to use ce->namelen, which would be a little faster than treating it > as a string (especially for strbuf, as it can then know up front how big > the size is). > > I doubt it's measurable, though (especially as the growth cost is > amortized due to the static buffer). I somehow feel that: strbuf_reset(&path_buf); strbuf_add(&path_buf, state->base_dir, state->base_dir_len); strbuf_addch(&path_buf, '/'); strbuf_add(&path_buf, state->name, state->name_len); feels a bit neater than using strbuf_addf. But that might just be me. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] entry.c: convert checkout_entry to use strbuf
On Wed, Oct 23, 2013 at 07:55:06PM +0700, Nguyen Thai Ngoc Duy wrote: > The old code does not do boundary check so any paths longer than > PATH_MAX can cause buffer overflow. Replace it with strbuf to handle > paths of arbitrary length. I think this is a reasonable solution. If we have such a long path, we are probably about to feed it to open() or another syscall, and we will just get ENAMETOOLONG there anyway. But certainly we need to fix the buffer overflow, and we are probably better off letting the syscall report failure than calling die(), because we generally handle the syscall failure more gracefully (e.g., by reporting the failed path but continuing). > - memcpy(path, state->base_dir, len); > - strcpy(path + len, ce->name); > - len += ce_namelen(ce); > + strbuf_reset(&path_buf); > + strbuf_addf(&path_buf, "%.*s%s", state->base_dir_len, state->base_dir, > ce->name); > + path = path_buf.buf; > + len = path_buf.len; This is not something you introduced, but while we are here, you may want to use ce->namelen, which would be a little faster than treating it as a string (especially for strbuf, as it can then know up front how big the size is). I doubt it's measurable, though (especially as the growth cost is amortized due to the static buffer). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] entry.c: convert checkout_entry to use strbuf
On Wed, Oct 23, 2013 at 3:04 PM, Duy Nguyen wrote:
> On Wed, Oct 23, 2013 at 7:58 PM, Antoine Pelisse wrote:
>>> diff --git a/entry.c b/entry.c
>>> index acc892f..d955af5 100644
>>> --- a/entry.c
>>> +++ b/entry.c
>>> @@ -237,16 +237,18 @@ static int check_path(const char *path, int len,
>>> struct stat *st, int skiplen)
>>> int checkout_entry(struct cache_entry *ce,
>>>const struct checkout *state, char *topath)
>>> {
>>> - static char path[PATH_MAX + 1];
>>> + static struct strbuf path_buf = STRBUF_INIT;
>>> + char *path;
>>> struct stat st;
>>> - int len = state->base_dir_len;
>>> + int len;
>>>
>>> if (topath)
>>> return write_entry(ce, topath, state, 1);
>>>
>>> - memcpy(path, state->base_dir, len);
>>> - strcpy(path + len, ce->name);
>>> - len += ce_namelen(ce);
>>> + strbuf_reset(&path_buf);
>>
>> I think this is not required
>
> If you mean strbuf_reset, I think it is. path_buf is still static (I
> don't want to remove that because it'll add a lot more strbuf_release)
> so we can't be sure what it contains from the second checkout_entry()
> call.
Of course, I forgot about the static,
Thanks :-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] entry.c: convert checkout_entry to use strbuf
On Wed, Oct 23, 2013 at 7:58 PM, Antoine Pelisse wrote:
>> diff --git a/entry.c b/entry.c
>> index acc892f..d955af5 100644
>> --- a/entry.c
>> +++ b/entry.c
>> @@ -237,16 +237,18 @@ static int check_path(const char *path, int len,
>> struct stat *st, int skiplen)
>> int checkout_entry(struct cache_entry *ce,
>>const struct checkout *state, char *topath)
>> {
>> - static char path[PATH_MAX + 1];
>> + static struct strbuf path_buf = STRBUF_INIT;
>> + char *path;
>> struct stat st;
>> - int len = state->base_dir_len;
>> + int len;
>>
>> if (topath)
>> return write_entry(ce, topath, state, 1);
>>
>> - memcpy(path, state->base_dir, len);
>> - strcpy(path + len, ce->name);
>> - len += ce_namelen(ce);
>> + strbuf_reset(&path_buf);
>
> I think this is not required
If you mean strbuf_reset, I think it is. path_buf is still static (I
don't want to remove that because it'll add a lot more strbuf_release)
so we can't be sure what it contains from the second checkout_entry()
call.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] entry.c: convert checkout_entry to use strbuf
On Wed, Oct 23, 2013 at 2:55 PM, Nguyễn Thái Ngọc Duy wrote:
> The old code does not do boundary check so any paths longer than
> PATH_MAX can cause buffer overflow. Replace it with strbuf to handle
> paths of arbitrary length.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> To get this topic going again. These two patches kill PATH_MAX in
> entry.c and builtin/checkout-index.c
Thanks !
> diff --git a/entry.c b/entry.c
> index acc892f..d955af5 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -237,16 +237,18 @@ static int check_path(const char *path, int len, struct
> stat *st, int skiplen)
> int checkout_entry(struct cache_entry *ce,
>const struct checkout *state, char *topath)
> {
> - static char path[PATH_MAX + 1];
> + static struct strbuf path_buf = STRBUF_INIT;
> + char *path;
> struct stat st;
> - int len = state->base_dir_len;
> + int len;
>
> if (topath)
> return write_entry(ce, topath, state, 1);
>
> - memcpy(path, state->base_dir, len);
> - strcpy(path + len, ce->name);
> - len += ce_namelen(ce);
> + strbuf_reset(&path_buf);
I think this is not required
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
