patch, 5 partsgit-prompt: do not look for refs/stash in $GIT_DIR

> We have a global pointer pack_data pointing to the current
> pack we have open. Inside end_packfile we have two new
> pointers, old_p and new_p. The latter points to pack_data,
> and the former points to the new "installed" version of the
> packfile we get when we hand the file off to the regular
> sha1_file machinery. When then free old_p.
>
> Presumably the extra old_p pointer was there so that we
> could overwrite pack_data with new_p and still free old_p,
> but we don't do that. We just leave pack_data pointing to
> bogus memory, and don't overwrite it until we call
> start_packfile again (if ever).
>
> This can cause problems for our die routine, which calls
> end_packfile to clean things up. If we die at the wrong
> moment, we can end up looking at invalid memory in
> pack_data left after the last end_packfile().
>
> Instead, let's make sure we set pack_data to NULL after we
> free it, and make calling endfile() again with a NULL
> pack_data a noop (there is nothing to end).
>
> We can further make things less confusing by dropping old_p
> entirely, and moving new_p closer to its point of use.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Noticed while running fast-import under valgrind to debug the next
> commit. :)
>
>  fast-import.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/fast-import.c b/fast-import.c
> index d73f58c..f25a4ae 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -946,10 +946,12 @@ static void unkeep_all_packs(void)
>
>  static void end_packfile(void)
>  {
> -       struct packed_git *old_p = pack_data, *new_p;
> +       if (!pack_data)
> +               return;
>
>         clear_delta_base_cache();
>         if (object_count) {
> +               struct packed_git *new_p;
>                 unsigned char cur_pack_sha1[20];
>                 char *idx_name;
>                 int i;
> @@ -991,10 +993,11 @@ static void end_packfile(void)
>                 pack_id++;
>         }
>         else {
> -               close(old_p->pack_fd);
> -               unlink_or_warn(old_p->pack_name);
> +               close(pack_data->pack_fd);
> +               unlink_or_warn(pack_data->pack_name);
>         }
> -       free(old_p);
> +       free(pack_data);
> +       pack_data = NULL;
>
>         /* We can't carry a delta across packfiles. */
>         strbuf_release(&last_blob.data);
> --
> 2.1.0.346.ga0367b9
>

> When creating a new annotated tag, we sprintf the refname
> into a static-sized buffer. If we have an absurdly long
> tagname, like:
>
>   git init repo &&
>   cd repo &&
>   git commit --allow-empty -m foo &&
>   git tag -m message mytag &&
>   git fast-export mytag |
>   perl -lpe '/^tag/ and s/mytag/"a" x 8192/e' |
>   git fast-import <input
>
> we'll overflow the buffer. We can fix it by using a strbuf.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I'm not sure how easily exploitable this is. The buffer is on the stack,
> and we definitely demolish the return address. But we never actually
> return from the function, since lock_ref_sha1 will fail in such a case
> and die (it cannot succeed because the name is longer than PATH_MAX,
> which we check when concatenating it with $GIT_DIR).
>
> Still, there is no limit to the size of buffer you can feed it, so it's
> entirely possible you can overwrite something else and cause some
> mischief. So I wouldn't call it trivially exploitable, but I would not
> bet my servers that it is not (and of course it is easy to trigger if
> you can convince somebody to run fast-import a stream, so any remote
> helpers produce a potentially vulnerable situation).
>
>  fast-import.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/fast-import.c b/fast-import.c
> index f25a4ae..a1479e9 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1734,14 +1734,16 @@ static void dump_tags(void)
>         static const char *msg = "fast-import";
>         struct tag *t;
>         struct ref_lock *lock;
> -       char ref_name[PATH_MAX];
> +       struct strbuf ref_name = STRBUF_INIT;
>
>         for (t = first_tag; t; t = t->next_tag) {
> -               sprintf(ref_name, "tags/%s", t->name);
> -               lock = lock_ref_sha1(ref_name, NULL);
> +               strbuf_reset(&ref_name);
> +               strbuf_addf(&ref_name, "tags/%s", t->name);
> +               lock = lock_ref_sha1(ref_name.buf, NULL);
>                 if (!lock || write_ref_sha1(lock, t->sha1, msg) < 0)
> -                       failure |= error("Unable to update %s", ref_name);
> +                       failure |= error("Unable to update %s", ref_name.buf);
>         }
> +       strbuf_release(&ref_name);
>  }
>
>  static void dump_marks_helper(FILE *f,
> --
> 2.1.0.346.ga0367b9
>

> We can use lock_any_ref_for_update instead. Besides being
> more flexible, the only difference between the two is that
> lock_ref_sha1 does not allow "top-level" refs like
> "refs/foo" to be updated. However, we know that we do not
> have such a ref, because we explicitly add the "tags/"
> prefix ourselves.
>
> Note that we now must feed the whole name "refs/tags/X"
> instead of just "tags/X" to the function. As a result, our
> failure error message is uses the longer name. This is
> probably a good thing, though.
>
> As an interesting side note, if we forgot to switch this
> input to the function, the tests do not currently catch it.
> We import a tag "X" and then check that we can access it at
> "tags/X". If we accidentally created "tags/X" at the
> top-level $GIT_DIR instead of under "refs/", we would still
> find it due to our ref lookup procedure!
>
> We do not make such a mistake in this patch, of course, but
> while we're thinking about it, let's make the fast-import
> tests more robust by checking for fully qualified
> refnames.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> As I mentioned, I'd be OK with dropping this one in favor of just
> waiting for Ronnie's transaction patches to graduate.
>
>  fast-import.c          | 4 ++--
>  t/t9300-fast-import.sh | 8 ++++----
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fast-import.c b/fast-import.c
> index a1479e9..04a85a4 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1738,8 +1738,8 @@ static void dump_tags(void)
>
>         for (t = first_tag; t; t = t->next_tag) {
>                 strbuf_reset(&ref_name);
> -               strbuf_addf(&ref_name, "tags/%s", t->name);
> -               lock = lock_ref_sha1(ref_name.buf, NULL);
> +               strbuf_addf(&ref_name, "refs/tags/%s", t->name);
> +               lock = lock_any_ref_for_update(ref_name.buf, NULL, 0, NULL);
>                 if (!lock || write_ref_sha1(lock, t->sha1, msg) < 0)
>                         failure |= error("Unable to update %s", ref_name.buf);
>         }
> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
> index 5fc9ef2..f4c6673 100755
> --- a/t/t9300-fast-import.sh
> +++ b/t/t9300-fast-import.sh
> @@ -153,7 +153,7 @@ tag series-A
>  An annotated tag without a tagger
>  EOF
>  test_expect_success 'A: verify tag/series-A' '
> -       git cat-file tag tags/series-A >actual &&
> +       git cat-file tag refs/tags/series-A >actual &&
>         test_cmp expect actual
>  '
>
> @@ -165,7 +165,7 @@ tag series-A-blob
>  An annotated tag that annotates a blob.
>  EOF
>  test_expect_success 'A: verify tag/series-A-blob' '
> -       git cat-file tag tags/series-A-blob >actual &&
> +       git cat-file tag refs/tags/series-A-blob >actual &&
>         test_cmp expect actual
>  '
>
> @@ -232,8 +232,8 @@ EOF
>  test_expect_success \
>         'A: tag blob by sha1' \
>         'git fast-import <input &&
> -       git cat-file tag tags/series-A-blob-2 >actual &&
> -       git cat-file tag tags/series-A-blob-3 >>actual &&
> +       git cat-file tag refs/tags/series-A-blob-2 >actual &&
> +       git cat-file tag refs/tags/series-A-blob-3 >>actual &&
>         test_cmp expect actual'
>
>  test_tick
> --
> 2.1.0.346.ga0367b9

> I noticed that "git pack-refs --all" will pack a top-level ref like
> "refs/foo", but will not actually prune "$GIT_DIR/refs/foo". I do not
> see the point in having a policy not to pack "refs/foo" if "--all" is
> given. But even if we did have such a policy, this seems broken; we
> should either pack and prune, or leave them untouched. I don't see any
> indication that the existing behavior was intentional.
> 
> The problem is that pack-refs's prune_ref calls lock_ref_sha1, which
> enforces this "no toplevel" behavior. I am not sure there is any real
> point to this, given that most callers use lock_any_ref_for_update,
> which is exactly equivalent but without the toplevel check.
> 
> The first two patches deal with this by switching pack-refs to use
> lock_any_ref_for_update. This will conflict slightly with Ronnie's
> ref-transaction work, as he gets rid of lock_ref_sha1 entirely, and
> moves the code directly into prune_ref. This can be trivially resolved
> in favor of my patch, I think.
> 
> The third patch is a cleanup I noticed while looking around, and I think
> should not conflict with anyone (and is a good thing to do).
> 
> The last two are trickier. I wondered if we could get rid of
> lock_ref_sha1 entirely. After pack-refs, there are two callers:
> fast-import.c and walker.c. After converting the first, it occurred to
> me that Ronnie might be touching the same areas, and I see that yes,
> indeed, there's quite a bit of conflict (and he reaches the same end
> goal of dropping it entirely).
> 
> So in that sense I do not mind dropping the final two patches. Ronnie's
> endpoint is much better, moving to a ref_transaction. However, there is
> actually a buffer overflow in the existing code. Ronnie's series fixes
> it in a similar way (moving to a strbuf), and I'm fine with that
> endpoint. But given that the ref transaction code is not yet merged (and
> would certainly never be maint-track), I think it is worth applying the
> buffer overflow fix separately.
> 
> I think the final patch can probably be dropped, then. It is a clean-up,
> but one that we can just get when Ronnie's series is merged.
> 
>   [1/5]: git-prompt: do not look for refs/stash in $GIT_DIR
>   [2/5]: pack-refs: prune top-level refs like "refs/foo"
>   [3/5]: fast-import: clean up pack_data pointer in end_packfile
>   [4/5]: fast-import: fix buffer overflow in dump_tags
>   [5/5]: fast-import: stop using lock_ref_sha1

> >   [1/5]: git-prompt: do not look for refs/stash in $GIT_DIR
> >   [2/5]: pack-refs: prune top-level refs like "refs/foo"
> >   [3/5]: fast-import: clean up pack_data pointer in end_packfile
> >   [4/5]: fast-import: fix buffer overflow in dump_tags
> >   [5/5]: fast-import: stop using lock_ref_sha1
> 
> +1 on patches 1 and 2
> Patch 3 is outside of my area of competence
> +1 on patch 4, which looks trivially correct.
> +1 on patch 5, though I agree with peff that it can be omitted in
> deference to Ronnie's work.

> I noticed that "git pack-refs --all" will pack a top-level ref like
> "refs/foo", but will not actually prune "$GIT_DIR/refs/foo". I do not
> see the point in having a policy not to pack "refs/foo" if "--all" is
> given. But even if we did have such a policy, this seems broken; we
> should either pack and prune, or leave them untouched. I don't see any
> indication that the existing behavior was intentional.
>
> The problem is that pack-refs's prune_ref calls lock_ref_sha1, which
> enforces this "no toplevel" behavior. I am not sure there is any real
> point to this, given that most callers use lock_any_ref_for_update,
> which is exactly equivalent but without the toplevel check.
>
> The first two patches deal with this by switching pack-refs to use
> lock_any_ref_for_update. This will conflict slightly with Ronnie's
> ref-transaction work, as he gets rid of lock_ref_sha1 entirely, and
> moves the code directly into prune_ref. This can be trivially resolved
> in favor of my patch, I think.
>
> The third patch is a cleanup I noticed while looking around, and I think
> should not conflict with anyone (and is a good thing to do).
>
> The last two are trickier. I wondered if we could get rid of
> lock_ref_sha1 entirely. After pack-refs, there are two callers:
> fast-import.c and walker.c. After converting the first, it occurred to
> me that Ronnie might be touching the same areas, and I see that yes,
> indeed, there's quite a bit of conflict (and he reaches the same end
> goal of dropping it entirely).
>
> So in that sense I do not mind dropping the final two patches. Ronnie's
> endpoint is much better, moving to a ref_transaction. However, there is
> actually a buffer overflow in the existing code. Ronnie's series fixes
> it in a similar way (moving to a strbuf), and I'm fine with that
> endpoint. But given that the ref transaction code is not yet merged (and
> would certainly never be maint-track), I think it is worth applying the
> buffer overflow fix separately.
>
> I think the final patch can probably be dropped, then. It is a clean-up,
> but one that we can just get when Ronnie's series is merged.
>
>   [1/5]: git-prompt: do not look for refs/stash in $GIT_DIR
>   [2/5]: pack-refs: prune top-level refs like "refs/foo"
>   [3/5]: fast-import: clean up pack_data pointer in end_packfile
>   [4/5]: fast-import: fix buffer overflow in dump_tags
>   [5/5]: fast-import: stop using lock_ref_sha1
>

> >   [1/5]: git-prompt: do not look for refs/stash in $GIT_DIR
> >   [2/5]: pack-refs: prune top-level refs like "refs/foo"
> >   [3/5]: fast-import: clean up pack_data pointer in end_packfile
> >   [4/5]: fast-import: fix buffer overflow in dump_tags
> >   [5/5]: fast-import: stop using lock_ref_sha1
> >
> 
> +1 on 1-3
> +1 on 4. While I have a similar fix in the transaction series, you
> should not need to wait for that series to address a security concern.
> 5: I think this one is not as urgent as the others so would prefer if
> it is dropped, just so it doesn't cause more merge conflicts than is
> already present in the transaction series.