fix pack-refs pruning of refs/foo

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 <[email protected]>

---
Noticed while running fast-import under valgrind to debug the next commit. :)

 fast-import.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

fast-import.c +7 -5

index d73f58c..f25a4ae 100644
@@ -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

Print an error before returning when pack_data is NULL ?

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 <[email protected]>

---
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(-)

fast-import.c +6 -5

index f25a4ae..a1479e9 100644
@@ -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 <[email protected]>

---
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(-)

fast-import.c +2 -2

index a1479e9..04a85a4 100644
@@ -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);
        }

t/t9300-fast-import.sh +4 -5

index 5fc9ef2..f4c6673 100755
@@ -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

Quoted 5 lines

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

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

Quoted 6 lines

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