[PATCH 0/5] fix pack-refs pruning of refs/foo

[PATCH 0/5] fix pack-refs pruning of refs/foo

[Jeff King]

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

-Peff

[PATCH 1/5] git-prompt: do not look for refs/stash in $GIT_DIR

[Jeff King]

Since dd0b72c (bash prompt: use bash builtins to check stash
state, 2011-04-01), git-prompt checks whether we have a
stash by looking for $GIT_DIR/refs/stash. Generally external
programs should never do this, because they would miss
packed-refs.

That commit claims that packed-refs does not pack
refs/stash, but that is not quite true. It does pack the
ref, but due to a bug, fails to prune the ref. When we fix
that bug, we would want to be doing the right thing here.

Signed-off-by: Jeff King <peff@peff.net>
---
I know we are pretty sensitive to forks in the prompt code (after all,
that was the point of dd0b72c). This patch is essentially a reversion of
this hunk of dd0b72c, and is definitely safe.

I think we could probably get by with:

  [ -r "$g/logs/ref/stash" ]

since reflogs are always in the filesystem. However, that seems somewhat
short-sighted, as the work Ronnie is doing is moving in the direction of
more abstraction here. I hope a day will come soon when reflogs do not
have to be stored in $GIT_DIR/logs, and then we would end up updating
this code again.

 contrib/completion/git-prompt.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 9d684b1..c5473dc 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -468,7 +468,8 @@ __git_ps1 ()
 			fi
 		fi
 		if [ -n "${GIT_PS1_SHOWSTASHSTATE-}" ] &&
-		   [ -r "$g/refs/stash" ]; then
+		   git rev-parse --verify --quiet refs/stash >/dev/null
+		then
 			s="$"
 		fi
 
-- 
2.1.0.346.ga0367b9

[PATCH 2/5] pack-refs: prune top-level refs like "refs/foo"

[Jeff King]

After we have packed all refs, we prune any loose refs that
correspond to what we packed. We do so by first taking a
lock with lock_ref_sha1, and then deleting the loose ref
file.

However, lock_ref_sha1 will refuse to take a lock on any
refs that exist at the top-level of the "refs/" directory,
and we skip pruning the ref.  This is almost certainly not
what we want to happen here. The criteria to be pruned
should not differ from that to be packed; if a ref makes it
to prune_ref, it's because we want it both packed and
pruned (if there are refs you do not want to be packed, they
should be omitted much earlier by pack_ref_is_possible,
which we do in this case if --all is not given).

We can fix this by switching to lock_any_ref_for_update.
This behaves exactly the same with the exception of this
top-level check.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs.c               | 3 ++-
 t/t3210-pack-refs.sh | 7 +++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 27927f2..82e5b1b 100644
--- a/refs.c
+++ b/refs.c
@@ -2387,7 +2387,8 @@ static void try_remove_empty_parents(char *name)
 /* make sure nobody touched the ref, and unlink */
 static void prune_ref(struct ref_to_prune *r)
 {
-	struct ref_lock *lock = lock_ref_sha1(r->name + 5, r->sha1);
+	struct ref_lock *lock = lock_any_ref_for_update(r->name, r->sha1,
+							0, NULL);
 
 	if (lock) {
 		unlink_or_warn(git_path("%s", r->name));
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 1a2080e..3a017bf 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -151,4 +151,11 @@ test_expect_success 'delete ref while another dangling packed ref' '
 	test_cmp /dev/null result
 '
 
+test_expect_success 'pack ref directly below refs/' '
+	git update-ref refs/top HEAD &&
+	git pack-refs --all --prune &&
+	grep refs/top .git/packed-refs &&
+	test_path_is_missing .git/refs/top
+'
+
 test_done
-- 
2.1.0.346.ga0367b9

[PATCH 3/5] fast-import: clean up pack_data pointer in end_packfile

[Jeff King]

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

Re: [PATCH 3/5] fast-import: clean up pack_data pointer in end_packfile

[Ronnie Sahlberg]

Print an error before returning when pack_data is NULL ?

Otherwise, LGTM

On Fri, Aug 22, 2014 at 10:27 PM, Jeff King <peff@peff.net> wrote:
> 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
>

Re: [PATCH 3/5] fast-import: clean up pack_data pointer in end_packfile

[Jeff King]

On Mon, Aug 25, 2014 at 10:16:52AM -0700, Ronnie Sahlberg wrote:

> Print an error before returning when pack_data is NULL ?

I don't think so. We call end_packfile in some code paths (like the die
handler) as "close if it's open". So I think it makes sense for it to be
a noop if nothing is open.

-Peff

[PATCH 4/5] fast-import: fix buffer overflow in dump_tags

[Jeff King]

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

Re: [PATCH 4/5] fast-import: fix buffer overflow in dump_tags

[Ronnie Sahlberg]

Jeff,
We have a fix like this in the next set of transaction updates.
https://code-review.googlesource.com/#/c/1012/13/fast-import.c

However, if your concerns are the integrity of the servers and not
taking any chances
you might not want to wait for my patches to graduate.


ronnie sahlberg

On Fri, Aug 22, 2014 at 10:32 PM, Jeff King <peff@peff.net> wrote:
> 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
>

[PATCH 5/5] fast-import: stop using lock_ref_sha1

[Jeff King]

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

Re: [PATCH 5/5] fast-import: stop using lock_ref_sha1

[Ronnie Sahlberg]

The next ref transaction series does a similar change and ends up
removing the function lock_ref_sha1() completely.
https://code-review.googlesource.com/#/c/1017/19/refs.c

So I think we can drop this patch.


ronnie sahlberg


On Fri, Aug 22, 2014 at 10:33 PM, Jeff King <peff@peff.net> wrote:
> 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

Re: [PATCH 0/5] fix pack-refs pruning of refs/foo

[Michael Haggerty]

On 08/23/2014 07:23 AM, Jeff King wrote:
> 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 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.

By the way, while cleaning up in patch 5 you might take the chance to
rename the local variable ref_name to refname to be consistent with most
of our code, but this is by no means required.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH 0/5] fix pack-refs pruning of refs/foo

[Jeff King]

On Sat, Aug 23, 2014 at 09:29:36AM +0200, Michael Haggerty wrote:

> >   [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.

Thanks.

> By the way, while cleaning up in patch 5 you might take the chance to
> rename the local variable ref_name to refname to be consistent with most
> of our code, but this is by no means required.

I had the exact same inclination, but dismissed it as me being too
picky. :)

I'll change it if I re-roll, but I think we'll end up just dropping that
patch entirely.

-Peff

Re: [PATCH 0/5] fix pack-refs pruning of refs/foo

[Ronnie Sahlberg]

On Fri, Aug 22, 2014 at 10:23 PM, Jeff King <peff@peff.net> wrote:
> 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 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.


1-4:
Reviewed-by: Ronnie Sahlberg <sahlberg@google.com>


> -Peff

Re: [PATCH 0/5] fix pack-refs pruning of refs/foo

[Jeff King]

On Mon, Aug 25, 2014 at 10:38:56AM -0700, Ronnie Sahlberg wrote:

> >   [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.

OK, I think we're in agreement then. Thanks for looking them over.

-Peff

Re: [PATCH 1/5] git-prompt: do not look for refs/stash in $GIT_DIR

[Gábor Szeder]

Hi,

On Aug 23, 2014 12:26 PM, Jeff King <peff@peff.net> wrote:
> Since dd0b72c (bash prompt: use bash builtins to check stash 
> state, 2011-04-01), git-prompt checks whether we have a 
> stash by looking for $GIT_DIR/refs/stash. Generally external 
> programs should never do this, because they would miss 
> packed-refs.

Not sure whether the prompt script is external program or not, but doesn't matter, this is the right thing to do.

> That commit claims that packed-refs does not pack 
> refs/stash, but that is not quite true. It does pack the 
> ref, but due to a bug, fails to prune the ref. When we fix 
> that bug, we would want to be doing the right thing here. 
>
> Signed-off-by: Jeff King <peff@peff.net> 
> --- 
> I know we are pretty sensitive to forks in the prompt code (after all, 
> that was the point of dd0b72c). This patch is essentially a reversion of 
> this hunk of dd0b72c, and is definitely safe.

I'm not sure, but if I remember correctly (don't have the means to check it at the moment, sorry) in that commit I also added a 'git pack-ref' invocation to the relevant test(s?) to guard us against breakages due to changes in 'git pack-refs'.  If that is so, then I think those invocations should be removed as well, as they'll become useless.

Best,
Gábor

Re: [PATCH 1/5] git-prompt: do not look for refs/stash in $GIT_DIR

[Jeff King]

On Sun, Aug 24, 2014 at 08:22:41PM +0700, Gábor Szeder wrote:

> On Aug 23, 2014 12:26 PM, Jeff King <peff@peff.net> wrote:
> > Since dd0b72c (bash prompt: use bash builtins to check stash 
> > state, 2011-04-01), git-prompt checks whether we have a 
> > stash by looking for $GIT_DIR/refs/stash. Generally external 
> > programs should never do this, because they would miss 
> > packed-refs.
> 
> Not sure whether the prompt script is external program or not, but
> doesn't matter, this is the right thing to do.

Yeah, by external I just meant "nothing outside of refs.c should make
this assumption".

> > That commit claims that packed-refs does not pack 
> > refs/stash, but that is not quite true. It does pack the 
> > ref, but due to a bug, fails to prune the ref. When we fix 
> > that bug, we would want to be doing the right thing here. 
> >
> > Signed-off-by: Jeff King <peff@peff.net> 
> > --- 
> > I know we are pretty sensitive to forks in the prompt code (after all, 
> > that was the point of dd0b72c). This patch is essentially a reversion of 
> > this hunk of dd0b72c, and is definitely safe.
> 
> I'm not sure, but if I remember correctly (don't have the means to
> check it at the moment, sorry) in that commit I also added a 'git
> pack-ref' invocation to the relevant test(s?) to guard us against
> breakages due to changes in 'git pack-refs'.  If that is so, then I
> think those invocations should be removed as well, as they'll become
> useless.

It did add that change (that's actually how I noticed the problem!
Thank you for being thorough in dd0b72c). My inclination is to leave the
pack-refs invocations, as they protect against a certain class of errors
(we are not doing the risky behavior now, but the purpose of the test
suite is to detect regressions; the next person to touch that code may
not be so careful as you were).

I don't feel too strongly, though, so if we want them gone, I'm OK with
that.

-Peff