Re: [PATCH 66/67] use strbuf_complete to conditionally append slash

[Jeff King]

On Sun, Sep 20, 2015 at 09:50:28PM -0400, Eric Sunshine wrote:

> > diff --git a/builtin/clean.c b/builtin/clean.c
> > index df53def..d7acb94 100644
> > --- a/builtin/clean.c
> > +++ b/builtin/clean.c
> > @@ -159,8 +159,7 @@ static int is_git_repository(struct strbuf *path)
> > int gitfile_error;
> > size_t orig_path_len = path->len;
> > assert(orig_path_len != 0);
> > -   if (path->buf[orig_path_len - 1] != '/')
> > -   strbuf_addch(path, '/');
> > +   strbuf_complete(path, '/');
> 
> Does the above assert() still have value following this change? I
> recall, when reviewing this code, specifically asking[1,2] for an
> assert() or some other check to show that accessing buf[orig_path_len
> - 1] was safe. Since this patch removes the code in question, the
> assert() may no longer have meaning.
> 
> [1]: 
> http://thread.gmane.org/gmane.comp.version-control.git/266839/focus=266892
> [2]: 
> http://thread.gmane.org/gmane.comp.version-control.git/266839/focus=266974

I didn't dig that far, as I was mostly aiming for an obvious
no-behavior-change transition to the new helper, and dropping the assert
means we will behave differently overall for an empty path.

I agree from the messages you quote that it is probably fine, but I
wonder if it should be in a separate patch.

-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 66/67] use strbuf_complete to conditionally append slash

[Eric Sunshine]

On Tue, Sep 15, 2015 at 12:16 PM, Jeff King  wrote:
> When working with paths in strbufs, we frequently want to
> ensure that a directory contains a trailing slash before
> appending to it. We can shorten this code (and make the
> intent more obvious) by calling strbuf_complete.
>
> Most of these cases are trivially identical conversions, but
> there are two things to note:
>
>   - in a few cases we did not check that the strbuf is
> non-empty (which would lead to an out-of-bounds memory
> access). These were generally not triggerable in
> practice, either from earlier assertions, or typically
> because we would have just fed the strbuf to opendir(),
> which would choke on an empty path.
>
>   - in a few cases we indexed the buffer with "original_len"
> or similar, rather than the current sb->len, and it is
> not immediately obvious from the diff that they are the
> same. In all of these cases, I manually verified that
> the strbuf does not change between the assignment and
> the strbuf_complete call.
>
> This does not convert cases which look like:
>
>   if (sb->len && !is_dir_sep(sb->buf[sb->len - 1]))
>   strbuf_addch(sb, '/');
>
> as those are obviously semantically different. Some of these
> cases arguably should be doing that, but that is out of
> scope for this change, which aims purely for cleanup with no
> behavior change (and at least it will make such sites easier
> to find and examine in the future, as we can grep for
> strbuf_complete).
>
> Signed-off-by: Jeff King 
> ---
> diff --git a/builtin/clean.c b/builtin/clean.c
> index df53def..d7acb94 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -159,8 +159,7 @@ static int is_git_repository(struct strbuf *path)
> int gitfile_error;
> size_t orig_path_len = path->len;
> assert(orig_path_len != 0);
> -   if (path->buf[orig_path_len - 1] != '/')
> -   strbuf_addch(path, '/');
> +   strbuf_complete(path, '/');

Does the above assert() still have value following this change? I
recall, when reviewing this code, specifically asking[1,2] for an
assert() or some other check to show that accessing buf[orig_path_len
- 1] was safe. Since this patch removes the code in question, the
assert() may no longer have meaning.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/266839/focus=266892
[2]: http://thread.gmane.org/gmane.comp.version-control.git/266839/focus=266974

> strbuf_addstr(path, ".git");
> if (read_gitfile_gently(path->buf, _error) || 
> is_git_directory(path->buf))
> ret = 1;
--
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 66/67] use strbuf_complete to conditionally append slash

[Junio C Hamano]

Jeff King  writes:

> On Wed, Sep 16, 2015 at 03:54:50PM -0700, Junio C Hamano wrote:
>
>> Jeff King  writes:
>> 
>> >> Is this conversion correct?  This seems to me that the caller wants
>> >> to create an IMAP folder name immediately under the root hierarchy
>> >> and wants to have the leading slash in the result.
>> >
>> > Ugh, you're right. This is the "other" style Eric mentioned earlier.
>> >
>> > This looks like the only one in the patch (there are many that did not
>> > check buf.len at all, but if we assume they were not invoking undefined
>> > behavior before, then they are fine under the new code).
>> 
>> Yes, I should have said that earlier to save one roundtrip.
>> 
>> Thanks for working on this.
>
> For my re-roll, I've just omitted changing that caller. I think we can
> leave it as-is; it is not worth trying to introduce a new helper for the
> one site.

Yup, I think the decision is sensible.

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 66/67] use strbuf_complete to conditionally append slash

[Junio C Hamano]

Jeff King  writes:

> diff --git a/imap-send.c b/imap-send.c
> index 01aa227..f5d2b06 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -1412,8 +1412,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
>   curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);
>  
>   strbuf_addstr(, server.host);
> - if (!path.len || path.buf[path.len - 1] != '/')
> - strbuf_addch(, '/');
> + strbuf_complete(, '/');
>   strbuf_addstr(, server.folder);

Is this conversion correct?  This seems to me that the caller wants
to create an IMAP folder name immediately under the root hierarchy
and wants to have the leading slash in the result.
--
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 66/67] use strbuf_complete to conditionally append slash

[Jeff King]

On Wed, Sep 16, 2015 at 03:54:50PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> >> Is this conversion correct?  This seems to me that the caller wants
> >> to create an IMAP folder name immediately under the root hierarchy
> >> and wants to have the leading slash in the result.
> >
> > Ugh, you're right. This is the "other" style Eric mentioned earlier.
> >
> > This looks like the only one in the patch (there are many that did not
> > check buf.len at all, but if we assume they were not invoking undefined
> > behavior before, then they are fine under the new code).
> 
> Yes, I should have said that earlier to save one roundtrip.
> 
> Thanks for working on this.

For my re-roll, I've just omitted changing that caller. I think we can
leave it as-is; it is not worth trying to introduce a new helper for the
one site.

-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 66/67] use strbuf_complete to conditionally append slash

[Jeff King]

On Wed, Sep 16, 2015 at 03:18:59PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > diff --git a/imap-send.c b/imap-send.c
> > index 01aa227..f5d2b06 100644
> > --- a/imap-send.c
> > +++ b/imap-send.c
> > @@ -1412,8 +1412,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
> > curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);
> >  
> > strbuf_addstr(, server.host);
> > -   if (!path.len || path.buf[path.len - 1] != '/')
> > -   strbuf_addch(, '/');
> > +   strbuf_complete(, '/');
> > strbuf_addstr(, server.folder);
> 
> Is this conversion correct?  This seems to me that the caller wants
> to create an IMAP folder name immediately under the root hierarchy
> and wants to have the leading slash in the result.

Ugh, you're right. This is the "other" style Eric mentioned earlier.

This looks like the only one in the patch (there are many that did not
check buf.len at all, but if we assume they were not invoking undefined
behavior before, then they are fine under the new code).

-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 66/67] use strbuf_complete to conditionally append slash

[Junio C Hamano]

Jeff King  writes:

>> Is this conversion correct?  This seems to me that the caller wants
>> to create an IMAP folder name immediately under the root hierarchy
>> and wants to have the leading slash in the result.
>
> Ugh, you're right. This is the "other" style Eric mentioned earlier.
>
> This looks like the only one in the patch (there are many that did not
> check buf.len at all, but if we assume they were not invoking undefined
> behavior before, then they are fine under the new code).

Yes, I should have said that earlier to save one roundtrip.

Thanks for working on this.
--
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