On Mon, Oct 3, 2016 at 1:34 PM, Jeff King <peff@xxxxxxxx> wrote:
> When we add a new alternate to the list, we try to normalize
> out any redundant "..", etc. However, we do not look at the
> return value of normalize_path_copy(), and will happily
> continue with a path that could not be normalized. Worse,
> the normalizing process is done in-place, so we are left
> with whatever half-finished working state the normalizing
> function was in.
>
> Fortunately, this cannot cause us to read past the end of
> our buffer, as that working state will always leave the
> NUL from the original path in place. And we do tend to
> notice problems when we check is_directory() on the path.
> But you can see the nonsense that we feed to is_directory
> with an entry like:
>
> this/../../is/../../way/../../too/../../deep/../../to/../../resolve
>
> in your objects/info/alternates, which yields:
>
> error: object directory
> /to/e/deep/too/way//ects/this/../../is/../../way/../../too/../../deep/../../to/../../resolve
> does not exist; check .git/objects/info/alternates.
>
Yikes, that doesn't seem helpful.
> We can easily fix this just by checking the return value.
> But that makes it hard to generate a good error message,
> since we're normalizing in-place and our input value has
> been overwritten by cruft.
Right. Definitely want to check the return value here...
>
> Instead, let's provide a strbuf helper that does an in-place
> normalize, but restores the original contents on error. This
> uses a second buffer under the hood, which is slightly less
> efficient, but this is not a performance-critical code path.
>
I agree, I don't think this duplication is really a big deal, since it
helps ensure that the function doesn't modify its arguments on error.
> The strbuf helper can also properly set the "len" parameter
> of the strbuf before returning. Just doing:
>
> normalize_path_copy(buf.buf, buf.buf);
>
> will shorten the string, but leave buf.len at the original
> length. That may be confusing to later code which uses the
> strbuf.
>
Makes sense here. Properly setting the length will help prevent future issues.
Thanks,
Jake
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> ---
> sha1_file.c | 11 +++++++++--
> strbuf.c | 20 ++++++++++++++++++++
> strbuf.h | 8 ++++++++
> 3 files changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index b9c1fa3..68571bd 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -263,7 +263,12 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,
> }
> strbuf_addstr(&pathbuf, entry);
>
> - normalize_path_copy(pathbuf.buf, pathbuf.buf);
> + if (strbuf_normalize_path(&pathbuf) < 0) {
> + error("unable to normalize alternate object path: %s",
> + pathbuf.buf);
> + strbuf_release(&pathbuf);
> + return -1;
> + }
>
> pfxlen = strlen(pathbuf.buf);
>
> @@ -335,7 +340,9 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,
> }
>
> strbuf_add_absolute_path(&objdirbuf, get_object_directory());
> - normalize_path_copy(objdirbuf.buf, objdirbuf.buf);
> + if (strbuf_normalize_path(&objdirbuf) < 0)
> + die("unable to normalize object directory: %s",
> + objdirbuf.buf);
>
> alt_copy = xmemdupz(alt, len);
> string_list_split_in_place(&entries, alt_copy, sep, -1);
> diff --git a/strbuf.c b/strbuf.c
> index b839be4..8fec657 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -870,3 +870,23 @@ void strbuf_stripspace(struct strbuf *sb, int skip_comments)
>
> strbuf_setlen(sb, j);
> }
> +
> +int strbuf_normalize_path(struct strbuf *src)
> +{
> + struct strbuf dst = STRBUF_INIT;
> +
> + strbuf_grow(&dst, src->len);
> + if (normalize_path_copy(dst.buf, src->buf) < 0) {
> + strbuf_release(&dst);
> + return -1;
> + }
> +
> + /*
> + * normalize_path does not tell us the new length, so we have to
> + * compute it by looking for the new NUL it placed
> + */
And we can't correctly set the length inside normalize_path_copy
because it just takes C strings directly and not actually a strbuf. Ok
so it makes sense that we have to set it here.
Thanks,
Jake
> + strbuf_setlen(&dst, strlen(dst.buf));
> + strbuf_swap(src, &dst);
> + strbuf_release(&dst);
> + return 0;
> +}
> diff --git a/strbuf.h b/strbuf.h
> index ba8d5f1..2262b12 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -443,6 +443,14 @@ extern int strbuf_getcwd(struct strbuf *sb);
> */
> extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);
>