# [PATCH 1/2] Documentation/technical/bitmap-format.txt: add missing position table 12 messages from 2024-06-06 to 2024-06-14. Participants: Taylor Blau, Junio C Hamano, Elijah Newren. Thread: https://gitlist.dev/t/61600 ## Taylor Blau, 2024-06-06 18:40 Subject: [PATCH 1/2] Documentation/technical/bitmap-format.txt: add missing position table Message-ID: URL: https://gitlist.dev/e/a71ec05e5dc0c8c40e1cce14a7c5fe946437a24d.1717699237.git.me%40ttaylorr.com ``` While investigating a benign Coverity warning on the new pseudo-merge implementation, I was struggling to understand the (paraphrased) below: ofs = index_end - 24 - (index->pseudo_merges.nr * sizeof(uint64_t)); for (i = 0; i < index->pseudo_merges.nr; i++) { index->pseudo_merges.v[i].at = get_be64(ofs); ofs += sizeof(uint64_t); } , in pack-bitmap.c::load_bitmap_header(). Looking at the documentation, the diagram describing the on-disk format (prior to this patch) suggested that the optional extended lookup table immediately preceded the trailing metadata portion. If that were the case, that would make the above code from load_bitmap_header() incorrect, as we'd be blindly reading into the extended offset table. But later on in the documentation there is a description of the pseudo-merge position table as immediately preceding the trailing metadata portion of the extension. And indeed, we do write the position table in pack-bitmap-write.c: /* write positions for all pseudo merges */ for (i = 0; i < writer->pseudo_merges_nr; i++) hashwrite_be64(f, pseudo_merge_ofs[i]); hashwrite_be32(f, writer->pseudo_merges_nr); hashwrite_be32(f, kh_size(writer->pseudo_merge_commits)); hashwrite_be64(f, table_start - start); hashwrite_be64(f, hashfile_total(f) - start + sizeof(uint64_t)); So this is purely a case of the diagram being out of sync with the textual description and actual implementation of the format specification. Add the missing component back to the format diagram to avoid further confusion in this area. Signed-off-by: Taylor Blau --- Documentation/technical/bitmap-format.txt | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/Documentation/technical/bitmap-format.txt b/Documentation/technical/bitmap-format.txt index ee7775a258..bfb0ec7beb 100644 --- a/Documentation/technical/bitmap-format.txt +++ b/Documentation/technical/bitmap-format.txt @@ -312,6 +312,15 @@ the end of a `.bitmap` file. The format is as follows: | | +-------------------------------------------+ | | +| Pseudo-merge position table | +| +----+----------+----------+----------+ | +| | N | Offset 1 | .... | Offset N | | +| +----+----------+----------+----------+ | +| | | 8 bytes | .... | 8 bytes | | +| +----+----------+----------+----------+ | +| | ++-------------------------------------------+ +| | | Pseudo-merge Metadata | | +-----------------------------------+ | | | # pseudo-merges (4 bytes) | | base-commit: 0b7500dc66ffcb6b1ccc3332715936a59c6b5ce4 -- 2.45.0.32.ga71ec05e5dc.dirty ``` ## Taylor Blau, 2024-06-06 18:41 Subject: [PATCH 2/2] pack-bitmap.c: ensure pseudo-merge offset reads are bounded Message-ID: <0a16399d14afd527f4db63f2a4a3b0a3cbf112f1.1717699237.git.me@ttaylorr.com> URL: https://gitlist.dev/e/0a16399d14afd527f4db63f2a4a3b0a3cbf112f1.1717699237.git.me%40ttaylorr.com In-Reply-To: ``` After reading the pseudo-merge extension's metadata table, we allocate an array to store information about each pseudo-merge, including its byte offset within the .bitmap file itself. This is done like so: pseudo_merge_ofs = index_end - 24 - (index->pseudo_merges.nr * sizeof(uint64_t)); for (i = 0; i < index->pseudo_merges.nr; i++) { index->pseudo_merges.v[i].at = get_be64(pseudo_merge_ofs); pseudo_merge_ofs += sizeof(uint64_t); } But if the pseudo-merge table is corrupt, we'll keep calling get_be64() past the end of the pseudo-merge extension, potentially reading off the end of the mmap'd region. Prevent this by ensuring that we have at least `table_size - 24` many bytes available to read (subtracting 24 as the length of the metadata component). This is sufficient to prevent us from reading off the end of the pseudo-merge extension, and ensures that all of the get_be64() calls below are in bounds. Helped-by: Jeff King Signed-off-by: Taylor Blau --- pack-bitmap.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pack-bitmap.c b/pack-bitmap.c index 70230e2647..ad2635c025 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -238,6 +238,9 @@ static int load_bitmap_header(struct bitmap_index *index) index->pseudo_merges.commits_nr = get_be32(index_end - 20); index->pseudo_merges.nr = get_be32(index_end - 24); + if (st_mult(index->pseudo_merges.nr, sizeof(uint64_t)) > table_size - 24) + return error(_("corrupted bitmap index file, pseudo-merge table too short")); + CALLOC_ARRAY(index->pseudo_merges.v, index->pseudo_merges.nr); -- 2.45.0.32.ga71ec05e5dc.dirty ``` ## Junio C Hamano, 2024-06-06 19:42 Subject: Re: [PATCH 2/2] pack-bitmap.c: ensure pseudo-merge offset reads are bounded Message-ID: URL: https://gitlist.dev/e/xmqqfrtpluws.fsf%40gitster.g In-Reply-To: <0a16399d14afd527f4db63f2a4a3b0a3cbf112f1.1717699237.git.me@ttaylorr.com> ``` Taylor Blau writes: > After reading the pseudo-merge extension's metadata table, we allocate > an array to store information about each pseudo-merge, including its > byte offset within the .bitmap file itself. > > This is done like so: > > pseudo_merge_ofs = index_end - 24 - > (index->pseudo_merges.nr * sizeof(uint64_t)); > for (i = 0; i < index->pseudo_merges.nr; i++) { > index->pseudo_merges.v[i].at = get_be64(pseudo_merge_ofs); > pseudo_merge_ofs += sizeof(uint64_t); > } > > But if the pseudo-merge table is corrupt, we'll keep calling get_be64() > past the end of the pseudo-merge extension, potentially reading off the > end of the mmap'd region. > > Prevent this by ensuring that we have at least `table_size - 24` many > bytes available to read (subtracting 24 as the length of the metadata > component). > > This is sufficient to prevent us from reading off the end of the > pseudo-merge extension, and ensures that all of the get_be64() calls > below are in bounds. Can table_size at this point be smaller than 24, which will allow (table_size - 24) to be a huge number that st_mult() will comfortably fit? > Helped-by: Jeff King > Signed-off-by: Taylor Blau > --- > pack-bitmap.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/pack-bitmap.c b/pack-bitmap.c > index 70230e2647..ad2635c025 100644 > --- a/pack-bitmap.c > +++ b/pack-bitmap.c > @@ -238,6 +238,9 @@ static int load_bitmap_header(struct bitmap_index *index) > index->pseudo_merges.commits_nr = get_be32(index_end - 20); > index->pseudo_merges.nr = get_be32(index_end - 24); > > + if (st_mult(index->pseudo_merges.nr, sizeof(uint64_t)) > table_size - 24) > + return error(_("corrupted bitmap index file, pseudo-merge table too short")); > + > CALLOC_ARRAY(index->pseudo_merges.v, > index->pseudo_merges.nr); ``` ## Taylor Blau, 2024-06-06 22:25 Subject: Re: [PATCH 2/2] pack-bitmap.c: ensure pseudo-merge offset reads are bounded Message-ID: URL: https://gitlist.dev/e/ZmI3RYSsx6VRHdc9%40nand.local In-Reply-To: ``` On Thu, Jun 06, 2024 at 12:42:59PM -0700, Junio C Hamano wrote: > Can table_size at this point be smaller than 24, which will allow > (table_size - 24) to be a huge number that st_mult() will > comfortably fit? It could be smaller than 24, but I think we're at the point of diminishing returns here. The table_size field is read from the .bitmap file itself, and we do some light bounds checking here: table_size = get_be64(index_end - 8); if (table_size > index_end - index->map - header_size) return error(_(...)); We could add another check to ensure that table_size is at least 24, but I'm less concerned here for a couple of reasons: - Since we're reading off of the index_end, we know that all of our reads are within the .bitmap itself, so we're not reading outside of the memory-mapped region. - Checking that index->pseudo_merges.nr is a reasonable size also bounds reads, but more importantly IMHO prevents a large heap allocation via the CALLOC_ARRAY() below. So I think we could check the table_size value, but I'm not sure we'd gain very much by doing so. Thanks, Taylor ``` ## Junio C Hamano, 2024-06-06 22:35 Subject: Re: [PATCH 2/2] pack-bitmap.c: ensure pseudo-merge offset reads are bounded Message-ID: URL: https://gitlist.dev/e/xmqqjzj1k8c8.fsf%40gitster.g In-Reply-To: ``` Taylor Blau writes: > On Thu, Jun 06, 2024 at 12:42:59PM -0700, Junio C Hamano wrote: >> Can table_size at this point be smaller than 24, which will allow >> (table_size - 24) to be a huge number that st_mult() will >> comfortably fit? > > It could be smaller than 24, but I think we're at the point of > diminishing returns here. I only meant to say that we could easily rewrite if (st_mult() > table_size - 24) condition to if (st_add(st_mult(), 24) > table_size) and we do not have to think if we have already checked table_size before we reach this point of the control flow. ``` ## Taylor Blau, 2024-06-06 22:38 Subject: Re: [PATCH 2/2] pack-bitmap.c: ensure pseudo-merge offset reads are bounded Message-ID: URL: https://gitlist.dev/e/ZmI6SqIFkWz8OPr1%40nand.local In-Reply-To: ``` On Thu, Jun 06, 2024 at 03:35:51PM -0700, Junio C Hamano wrote: > Taylor Blau writes: > > > On Thu, Jun 06, 2024 at 12:42:59PM -0700, Junio C Hamano wrote: > >> Can table_size at this point be smaller than 24, which will allow > >> (table_size - 24) to be a huge number that st_mult() will > >> comfortably fit? > > > > It could be smaller than 24, but I think we're at the point of > > diminishing returns here. > > I only meant to say that we could easily rewrite > > if (st_mult() > table_size - 24) > > condition to > > if (st_add(st_mult(), 24) > table_size) > > and we do not have to think if we have already checked table_size > before we reach this point of the control flow. Ah. Thanks for the clarification. Yes, I think you could do so; I'm happy to send another version if you like. Thanks, Taylor ``` ## Junio C Hamano, 2024-06-14 18:23 Subject: Re: [PATCH 2/2] pack-bitmap.c: ensure pseudo-merge offset reads are bounded Message-ID: URL: https://gitlist.dev/e/xmqqa5jnfko6.fsf%40gitster.g In-Reply-To: ``` Taylor Blau writes: > On Thu, Jun 06, 2024 at 03:35:51PM -0700, Junio C Hamano wrote: >> Taylor Blau writes: >> >> > On Thu, Jun 06, 2024 at 12:42:59PM -0700, Junio C Hamano wrote: >> >> Can table_size at this point be smaller than 24, which will allow >> >> (table_size - 24) to be a huge number that st_mult() will >> >> comfortably fit? >> > >> > It could be smaller than 24, but I think we're at the point of >> > diminishing returns here. >> >> I only meant to say that we could easily rewrite >> >> if (st_mult() > table_size - 24) >> >> condition to >> >> if (st_add(st_mult(), 24) > table_size) >> >> and we do not have to think if we have already checked table_size >> before we reach this point of the control flow. > > Ah. Thanks for the clarification. Yes, I think you could do so; I'm > happy to send another version if you like. Let's get this thing unstuck; the other larger topic that this topic builds upon is stuck for too long. ``` ## Taylor Blau, 2024-06-14 19:23 Subject: [PATCH v2 0/2] pseudo-merge: various small fixes Message-ID: URL: https://gitlist.dev/e/cover.1718392943.git.me%40ttaylorr.com In-Reply-To: ``` Here is a small reroll of a couple of patches I wrote to fix various small issues with the tb/pseudo-merge-reachability-bitmaps topic. The only change since last time is replacing: if (st_mult(index->pseudo_merges.nr, sizeof(uint64_t)) > table_size - 24) with: if (st_add(st_mult(index->pseudo_merges.nr, sizeof(uint64_t)), 24) > table_size) based on helpful review from Junio. For convenience, a range-diff is below. Thanks in advance for any final review on this topic :-). Taylor Blau (2): Documentation/technical/bitmap-format.txt: add missing position table pack-bitmap.c: ensure pseudo-merge offset reads are bounded Documentation/technical/bitmap-format.txt | 9 +++++++++ pack-bitmap.c | 5 +++++ 2 files changed, 14 insertions(+) Range-diff against v1: -: ---------- > 1: a71ec05e5d Documentation/technical/bitmap-format.txt: add missing position table 1: 0a16399d14 ! 2: 8abd564e7c pack-bitmap.c: ensure pseudo-merge offset reads are bounded @@ Commit message end of the mmap'd region. Prevent this by ensuring that we have at least `table_size - 24` many - bytes available to read (subtracting 24 as the length of the metadata - component). + bytes available to read (adding 24 to the left-hand side of our + inequality to account for the length of the metadata component). This is sufficient to prevent us from reading off the end of the pseudo-merge extension, and ensures that all of the get_be64() calls @@ pack-bitmap.c: static int load_bitmap_header(struct bitmap_index *index) index->pseudo_merges.commits_nr = get_be32(index_end - 20); index->pseudo_merges.nr = get_be32(index_end - 24); -+ if (st_mult(index->pseudo_merges.nr, sizeof(uint64_t)) > table_size - 24) ++ if (st_add(st_mult(index->pseudo_merges.nr, ++ sizeof(uint64_t)), ++ 24) > table_size) + return error(_("corrupted bitmap index file, pseudo-merge table too short")); + CALLOC_ARRAY(index->pseudo_merges.v, base-commit: 0b7500dc66ffcb6b1ccc3332715936a59c6b5ce4 -- 2.45.0.33.g0a16399d14.dirty ``` ## Taylor Blau, 2024-06-14 19:23 Subject: [PATCH v2 1/2] Documentation/technical/bitmap-format.txt: add missing position table Message-ID: URL: https://gitlist.dev/e/a71ec05e5dc0c8c40e1cce14a7c5fe946437a24d.1718392943.git.me%40ttaylorr.com In-Reply-To: ``` While investigating a benign Coverity warning on the new pseudo-merge implementation, I was struggling to understand the (paraphrased) below: ofs = index_end - 24 - (index->pseudo_merges.nr * sizeof(uint64_t)); for (i = 0; i < index->pseudo_merges.nr; i++) { index->pseudo_merges.v[i].at = get_be64(ofs); ofs += sizeof(uint64_t); } , in pack-bitmap.c::load_bitmap_header(). Looking at the documentation, the diagram describing the on-disk format (prior to this patch) suggested that the optional extended lookup table immediately preceded the trailing metadata portion. If that were the case, that would make the above code from load_bitmap_header() incorrect, as we'd be blindly reading into the extended offset table. But later on in the documentation there is a description of the pseudo-merge position table as immediately preceding the trailing metadata portion of the extension. And indeed, we do write the position table in pack-bitmap-write.c: /* write positions for all pseudo merges */ for (i = 0; i < writer->pseudo_merges_nr; i++) hashwrite_be64(f, pseudo_merge_ofs[i]); hashwrite_be32(f, writer->pseudo_merges_nr); hashwrite_be32(f, kh_size(writer->pseudo_merge_commits)); hashwrite_be64(f, table_start - start); hashwrite_be64(f, hashfile_total(f) - start + sizeof(uint64_t)); So this is purely a case of the diagram being out of sync with the textual description and actual implementation of the format specification. Add the missing component back to the format diagram to avoid further confusion in this area. Signed-off-by: Taylor Blau --- Documentation/technical/bitmap-format.txt | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/Documentation/technical/bitmap-format.txt b/Documentation/technical/bitmap-format.txt index ee7775a258..bfb0ec7beb 100644 --- a/Documentation/technical/bitmap-format.txt +++ b/Documentation/technical/bitmap-format.txt @@ -312,6 +312,15 @@ the end of a `.bitmap` file. The format is as follows: | | +-------------------------------------------+ | | +| Pseudo-merge position table | +| +----+----------+----------+----------+ | +| | N | Offset 1 | .... | Offset N | | +| +----+----------+----------+----------+ | +| | | 8 bytes | .... | 8 bytes | | +| +----+----------+----------+----------+ | +| | ++-------------------------------------------+ +| | | Pseudo-merge Metadata | | +-----------------------------------+ | | | # pseudo-merges (4 bytes) | | -- 2.45.0.33.g0a16399d14.dirty ``` ## Taylor Blau, 2024-06-14 19:23 Subject: [PATCH v2 2/2] pack-bitmap.c: ensure pseudo-merge offset reads are bounded Message-ID: <8abd564e7cbea8a3a165db86354d10a3f966d50f.1718392943.git.me@ttaylorr.com> URL: https://gitlist.dev/e/8abd564e7cbea8a3a165db86354d10a3f966d50f.1718392943.git.me%40ttaylorr.com In-Reply-To: ``` After reading the pseudo-merge extension's metadata table, we allocate an array to store information about each pseudo-merge, including its byte offset within the .bitmap file itself. This is done like so: pseudo_merge_ofs = index_end - 24 - (index->pseudo_merges.nr * sizeof(uint64_t)); for (i = 0; i < index->pseudo_merges.nr; i++) { index->pseudo_merges.v[i].at = get_be64(pseudo_merge_ofs); pseudo_merge_ofs += sizeof(uint64_t); } But if the pseudo-merge table is corrupt, we'll keep calling get_be64() past the end of the pseudo-merge extension, potentially reading off the end of the mmap'd region. Prevent this by ensuring that we have at least `table_size - 24` many bytes available to read (adding 24 to the left-hand side of our inequality to account for the length of the metadata component). This is sufficient to prevent us from reading off the end of the pseudo-merge extension, and ensures that all of the get_be64() calls below are in bounds. Helped-by: Jeff King Signed-off-by: Taylor Blau --- pack-bitmap.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pack-bitmap.c b/pack-bitmap.c index 70230e2647..ae13d7ee3b 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -238,6 +238,11 @@ static int load_bitmap_header(struct bitmap_index *index) index->pseudo_merges.commits_nr = get_be32(index_end - 20); index->pseudo_merges.nr = get_be32(index_end - 24); + if (st_add(st_mult(index->pseudo_merges.nr, + sizeof(uint64_t)), + 24) > table_size) + return error(_("corrupted bitmap index file, pseudo-merge table too short")); + CALLOC_ARRAY(index->pseudo_merges.v, index->pseudo_merges.nr); -- 2.45.0.33.g0a16399d14.dirty ``` ## Elijah Newren, 2024-06-14 21:08 Subject: Re: [PATCH v2 0/2] pseudo-merge: various small fixes Message-ID: URL: https://gitlist.dev/e/CABPp-BF71VWMmSHNx7-dbs8ek%2BdK0kPTUnSv2OJmM9VYq-LOkQ%40mail.gmail.com In-Reply-To: ``` On Fri, Jun 14, 2024 at 7:23 PM Taylor Blau wrote: > > Here is a small reroll of a couple of patches I wrote to fix various > small issues with the tb/pseudo-merge-reachability-bitmaps topic. > > The only change since last time is replacing: > > if (st_mult(index->pseudo_merges.nr, sizeof(uint64_t)) > table_size - 24) > > with: > > if (st_add(st_mult(index->pseudo_merges.nr, sizeof(uint64_t)), 24) > table_size) > > based on helpful review from Junio. For convenience, a range-diff is > below. Thanks in advance for any final review on this topic :-). > > Taylor Blau (2): > Documentation/technical/bitmap-format.txt: add missing position table > pack-bitmap.c: ensure pseudo-merge offset reads are bounded > > Documentation/technical/bitmap-format.txt | 9 +++++++++ > pack-bitmap.c | 5 +++++ > 2 files changed, 14 insertions(+) > > Range-diff against v1: > -: ---------- > 1: a71ec05e5d Documentation/technical/bitmap-format.txt: add missing position table > 1: 0a16399d14 ! 2: 8abd564e7c pack-bitmap.c: ensure pseudo-merge offset reads are bounded > @@ Commit message > end of the mmap'd region. > > Prevent this by ensuring that we have at least `table_size - 24` many > - bytes available to read (subtracting 24 as the length of the metadata > - component). > + bytes available to read (adding 24 to the left-hand side of our > + inequality to account for the length of the metadata component). > > This is sufficient to prevent us from reading off the end of the > pseudo-merge extension, and ensures that all of the get_be64() calls > @@ pack-bitmap.c: static int load_bitmap_header(struct bitmap_index *index) > index->pseudo_merges.commits_nr = get_be32(index_end - 20); > index->pseudo_merges.nr = get_be32(index_end - 24); > > -+ if (st_mult(index->pseudo_merges.nr, sizeof(uint64_t)) > table_size - 24) > ++ if (st_add(st_mult(index->pseudo_merges.nr, > ++ sizeof(uint64_t)), > ++ 24) > table_size) > + return error(_("corrupted bitmap index file, pseudo-merge table too short")); > + > CALLOC_ARRAY(index->pseudo_merges.v, > > base-commit: 0b7500dc66ffcb6b1ccc3332715936a59c6b5ce4 > -- > 2.45.0.33.g0a16399d14.dirty Series looks good to me. ``` ## Junio C Hamano, 2024-06-14 21:23 Subject: Re: [PATCH v2 0/2] pseudo-merge: various small fixes Message-ID: URL: https://gitlist.dev/e/xmqq1q4zdxrb.fsf%40gitster.g In-Reply-To: ``` Elijah Newren writes: > On Fri, Jun 14, 2024 at 7:23 PM Taylor Blau wrote: >> >> Here is a small reroll of a couple of patches I wrote to fix various >> small issues with the tb/pseudo-merge-reachability-bitmaps topic. >> > ... > Series looks good to me. Thanks, it looked good to me, too. Let's roll it into the base topic that has been waiting in 'next'. ```