pack-bitmap: fix pseudo-merge lookup for shared commits

When a commit appears in more than one pseudo-merge group, its entry in
the commit lookup table has the high bit set in its offset field,
indicating that the offset points to an "extended" table containing the
set of pseudo-merges for that commit.

There are three bugs in this path:

 * The `next_ext` offset in `write_pseudo_merges()` undercounts the
   per-entry size of the lookup table (8 vs. 12 bytes).

 * `nth_pseudo_merge_ext()` calls `read_pseudo_merge_commit_at()` on a
   pseudo-merge bitmap offset, misinterpreting it as a 12-byte commit
   table entry.

 * The error check after `pseudo_merge_ext_at()` in
   `apply_pseudo_merges_for_commit()` tests `< -1` instead of `< 0`,
   silently swallowing errors from `error()`.

The first bug is on the write side: each commit lookup entry contains a
4- and 8-byte unsigned value for a total of 12 bytes, but the
calculation assumes that the entry only contains 8 bytes of data. This
makes `next_ext` too small, so the extended-table offsets that get
written point into the middle of the non-extended lookup table rather
than past it. The reader then interprets non-extended lookup data as
extended entries, producing garbage.

The second bug is on the read side and is independently fatal: even with
a correctly positioned extended table, `nth_pseudo_merge_ext()` feeds
the offset it reads (which points at pseudo-merge bitmap data) to
`read_pseudo_merge_commit_at()`. That function tries to parse 12 bytes
as a `pseudo_merge_commit` struct, clobbering `merge->pseudo_merge_ofs`
with whatever happens to be at that location. The caller only needs
`pseudo_merge_ofs`, so the fix is to store the offset directly rather
than re-parsing a commit table entry. The `commit_pos` field is left
untouched, retaining the value that `find_pseudo_merge()` set earlier.

The third bug is latent. With the first two fixes applied, the extended
table is correctly written and read, so `pseudo_merge_ext_at()` does not
fail during normal operation. The `< -1` vs `< 0` distinction only
matters when the bitmap file is corrupt or truncated, in which case the
error would be silently ignored and the code would proceed with
uninitialized data.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index 86ed6a5d78cd04a116d7902bd9a7ecddeb282e93..1c8070f99c03ca9ce832a4ca47b5d9a66d526a29 100644 (file)
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -877,7 +877,7 @@ static void write_pseudo_merges(struct bitmap_writer *writer,
 
        next_ext = st_add(hashfile_total(f),
                          st_mult(kh_size(writer->pseudo_merge_commits),
-                                 sizeof(uint64_t)));
+                                 sizeof(uint32_t) + sizeof(uint64_t)));
 
        table_start = hashfile_total(f);
 

diff --git a/pseudo-merge.c b/pseudo-merge.c
index fb71c7617924a795853b8cc64239af239202051b..34e1da00b4e2002740c14c1e798d70328e34a180 100644 (file)
--- a/pseudo-merge.c
+++ b/pseudo-merge.c
@@ -600,7 +600,7 @@ static int nth_pseudo_merge_ext(const struct pseudo_merge_map *pm,
                return error(_("out-of-bounds read: (%"PRIuMAX" >= %"PRIuMAX")"),
                             (uintmax_t)ofs, (uintmax_t)pm->map_size);
 
-       read_pseudo_merge_commit_at(merge, pm->map + ofs);
+       merge->pseudo_merge_ofs = ofs;
 
        return 0;
 }
@@ -671,7 +671,7 @@ int apply_pseudo_merges_for_commit(const struct pseudo_merge_map *pm,
                off_t ofs = merge_commit.pseudo_merge_ofs & ~((uint64_t)1<<63);
                uint32_t i;
 
-               if (pseudo_merge_ext_at(pm, &ext, ofs) < -1) {
+               if (pseudo_merge_ext_at(pm, &ext, ofs) < 0) {
                        warning(_("could not read extended pseudo-merge table "
                                  "for commit %s"),
                                oid_to_hex(&commit->object.oid));

diff --git a/t/t5333-pseudo-merge-bitmaps.sh b/t/t5333-pseudo-merge-bitmaps.sh
index 5411fbf1e0451681b105002c6d16f7f8789c951a..90459da5e63c315213e7b820e593252b8b437fb3 100755 (executable)
--- a/t/t5333-pseudo-merge-bitmaps.sh
+++ b/t/t5333-pseudo-merge-bitmaps.sh
@@ -549,7 +549,7 @@ test_expect_success 'apply pseudo-merges from multiple groups during fill-in' '
        )
 '
 
-test_expect_failure 'apply pseudo-merges with overlapping groups during fill-in' '
+test_expect_success 'apply pseudo-merges with overlapping groups during fill-in' '
        test_when_finished "rm -fr pseudo-merge-fill-in-overlap" &&
        git init pseudo-merge-fill-in-overlap &&
        (