{"thread":{"id":"61600","subject":"[PATCH 1/2] Documentation/technical/bitmap-format.txt: add missing position table","startedAt":"2024-06-06T18:41:00Z","lastAt":"2024-06-14T21:23:41Z","messageCount":12,"participants":["Taylor Blau","Junio C Hamano","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"496557","messageId":"a71ec05e5dc0c8c40e1cce14a7c5fe946437a24d.1717699237.git.me@ttaylorr.com","threadId":"61600","inReplyTo":null,"subject":"[PATCH 1/2] Documentation/technical/bitmap-format.txt: add missing position table","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-06T18:40:52Z","receivedAt":"2024-06-06T18:41:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"While investigating a benign Coverity warning on the new pseudo-merge\nimplementation, I was struggling to understand the (paraphrased) below:\n\n ofs = index_end - 24 - (index->pseudo_merges.nr * sizeof(uint64_t));\n for (i = 0; i < index->pseudo_merges.nr; i++) {\n index->pseudo_merges.v[i].at = get_be64(ofs);\n ofs += sizeof(uint64_t);\n }\n\n, in pack-bitmap.c::load_bitmap_header(). Looking at the documentation,\nthe diagram describing the on-disk format (prior to this patch)\nsuggested that the optional extended lookup table immediately preceded\nthe trailing metadata portion.\n\nIf that were the case, that would make the above code from\nload_bitmap_header() incorrect, as we'd be blindly reading into the\nextended offset table.\n\nBut later on in the documentation there is a description of the\npseudo-merge position table as immediately preceding the trailing\nmetadata portion of the extension. And indeed, we do write the position\ntable in pack-bitmap-write.c:\n\n /* write positions for all pseudo merges */\n for (i = 0; i < writer->pseudo_merges_nr; i++)\n hashwrite_be64(f, pseudo_merge_ofs[i]);\n\n hashwrite_be32(f, writer->pseudo_merges_nr);\n hashwrite_be32(f, kh_size(writer->pseudo_merge_commits));\n hashwrite_be64(f, table_start - start);\n hashwrite_be64(f, hashfile_total(f) - start + sizeof(uint64_t));\n\nSo this is purely a case of the diagram being out of sync with the\ntextual description and actual implementation of the format\nspecification.\n\nAdd the missing component back to the format diagram to avoid further\nconfusion in this area.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/technical/bitmap-format.txt | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/Documentation/technical/bitmap-format.txt b/Documentation/technical/bitmap-format.txt\nindex ee7775a258..bfb0ec7beb 100644\n--- a/Documentation/technical/bitmap-format.txt\n+++ b/Documentation/technical/bitmap-format.txt\n@@ -312,6 +312,15 @@ the end of a `.bitmap` file. The format is as follows:\n | |\n +-------------------------------------------+\n | |\n+| Pseudo-merge position table |\n+| +----+----------+----------+----------+ |\n+| | N | Offset 1 | .... | Offset N | |\n+| +----+----------+----------+----------+ |\n+| | | 8 bytes | .... | 8 bytes | |\n+| +----+----------+----------+----------+ |\n+| |\n++-------------------------------------------+\n+| |\n | Pseudo-merge Metadata |\n | +-----------------------------------+ |\n | | # pseudo-merges (4 bytes) | |\n\nbase-commit: 0b7500dc66ffcb6b1ccc3332715936a59c6b5ce4\n-- \n2.45.0.32.ga71ec05e5dc.dirty\n\n"},{"id":"496558","messageId":"0a16399d14afd527f4db63f2a4a3b0a3cbf112f1.1717699237.git.me@ttaylorr.com","threadId":"61600","inReplyTo":"a71ec05e5dc0c8c40e1cce14a7c5fe946437a24d.1717699237.git.me@ttaylorr.com","subject":"[PATCH 2/2] pack-bitmap.c: ensure pseudo-merge offset reads are bounded","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-06T18:41:00Z","receivedAt":"2024-06-06T18:41:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"After reading the pseudo-merge extension's metadata table, we allocate\nan array to store information about each pseudo-merge, including its\nbyte offset within the .bitmap file itself.\n\nThis is done like so:\n\n pseudo_merge_ofs = index_end - 24 -\n (index->pseudo_merges.nr * sizeof(uint64_t));\n for (i = 0; i < index->pseudo_merges.nr; i++) {\n index->pseudo_merges.v[i].at = get_be64(pseudo_merge_ofs);\n pseudo_merge_ofs += sizeof(uint64_t);\n }\n\nBut if the pseudo-merge table is corrupt, we'll keep calling get_be64()\npast the end of the pseudo-merge extension, potentially reading off the\nend of the mmap'd region.\n\nPrevent this by ensuring that we have at least `table_size - 24` many\nbytes available to read (subtracting 24 as the length of the metadata\ncomponent).\n\nThis is sufficient to prevent us from reading off the end of the\npseudo-merge extension, and ensures that all of the get_be64() calls\nbelow are in bounds.\n\nHelped-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n pack-bitmap.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 70230e2647..ad2635c025 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -238,6 +238,9 @@ static int load_bitmap_header(struct bitmap_index *index)\n \t\t\t\tindex->pseudo_merges.commits_nr = get_be32(index_end - 20);\n \t\t\t\tindex->pseudo_merges.nr = get_be32(index_end - 24);\n \n+\t\t\t\tif (st_mult(index->pseudo_merges.nr, sizeof(uint64_t)) > table_size - 24)\n+\t\t\t\t\treturn error(_(\"corrupted bitmap index file, pseudo-merge table too short\"));\n+\n \t\t\t\tCALLOC_ARRAY(index->pseudo_merges.v,\n \t\t\t\t\t index->pseudo_merges.nr);\n \n-- \n2.45.0.32.ga71ec05e5dc.dirty\n"},{"id":"496560","messageId":"xmqqfrtpluws.fsf@gitster.g","threadId":"61600","inReplyTo":"0a16399d14afd527f4db63f2a4a3b0a3cbf112f1.1717699237.git.me@ttaylorr.com","subject":"Re: [PATCH 2/2] pack-bitmap.c: ensure pseudo-merge offset reads are bounded","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-06T19:42:59Z","receivedAt":"2024-06-06T19:43:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> After reading the pseudo-merge extension's metadata table, we allocate\n> an array to store information about each pseudo-merge, including its\n> byte offset within the .bitmap file itself.\n>\n> This is done like so:\n>\n> pseudo_merge_ofs = index_end - 24 -\n> (index->pseudo_merges.nr * sizeof(uint64_t));\n> for (i = 0; i < index->pseudo_merges.nr; i++) {\n> index->pseudo_merges.v[i].at = get_be64(pseudo_merge_ofs);\n> pseudo_merge_ofs += sizeof(uint64_t);\n> }\n>\n> But if the pseudo-merge table is corrupt, we'll keep calling get_be64()\n> past the end of the pseudo-merge extension, potentially reading off the\n> end of the mmap'd region.\n>\n> Prevent this by ensuring that we have at least `table_size - 24` many\n> bytes available to read (subtracting 24 as the length of the metadata\n> component).\n>\n> This is sufficient to prevent us from reading off the end of the\n> pseudo-merge extension, and ensures that all of the get_be64() calls\n> below are in bounds.\n\nCan table_size at this point be smaller than 24, which will allow\n(table_size - 24) to be a huge number that st_mult() will\ncomfortably fit?\n\n> Helped-by: Jeff King \n> Signed-off-by: Taylor Blau \n> ---\n> pack-bitmap.c | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index 70230e2647..ad2635c025 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -238,6 +238,9 @@ static int load_bitmap_header(struct bitmap_index *index)\n> \t\t\t\tindex->pseudo_merges.commits_nr = get_be32(index_end - 20);\n> \t\t\t\tindex->pseudo_merges.nr = get_be32(index_end - 24);\n> \n> +\t\t\t\tif (st_mult(index->pseudo_merges.nr, sizeof(uint64_t)) > table_size - 24)\n> +\t\t\t\t\treturn error(_(\"corrupted bitmap index file, pseudo-merge table too short\"));\n> +\n> \t\t\t\tCALLOC_ARRAY(index->pseudo_merges.v,\n> \t\t\t\t\t index->pseudo_merges.nr);\n"},{"id":"496568","messageId":"ZmI3RYSsx6VRHdc9@nand.local","threadId":"61600","inReplyTo":"xmqqfrtpluws.fsf@gitster.g","subject":"Re: [PATCH 2/2] pack-bitmap.c: ensure pseudo-merge offset reads are bounded","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-06T22:25:09Z","receivedAt":"2024-06-06T22:25:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jun 06, 2024 at 12:42:59PM -0700, Junio C Hamano wrote:\n> Can table_size at this point be smaller than 24, which will allow\n> (table_size - 24) to be a huge number that st_mult() will\n> comfortably fit?\n\nIt could be smaller than 24, but I think we're at the point of\ndiminishing returns here. The table_size field is read from the .bitmap\nfile itself, and we do some light bounds checking here:\n\n table_size = get_be64(index_end - 8);\n if (table_size > index_end - index->map - header_size)\n return error(_(...));\n\nWe could add another check to ensure that table_size is at least 24, but\nI'm less concerned here for a couple of reasons:\n\n - Since we're reading off of the index_end, we know that all of our\n reads are within the .bitmap itself, so we're not reading outside of\n the memory-mapped region.\n\n - Checking that index->pseudo_merges.nr is a reasonable size also\n bounds reads, but more importantly IMHO prevents a large heap\n allocation via the CALLOC_ARRAY() below.\n\nSo I think we could check the table_size value, but I'm not sure we'd\ngain very much by doing so.\n\nThanks,\nTaylor\n"},{"id":"496569","messageId":"xmqqjzj1k8c8.fsf@gitster.g","threadId":"61600","inReplyTo":"ZmI3RYSsx6VRHdc9@nand.local","subject":"Re: [PATCH 2/2] pack-bitmap.c: ensure pseudo-merge offset reads are bounded","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-06T22:35:51Z","receivedAt":"2024-06-06T22:35:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Thu, Jun 06, 2024 at 12:42:59PM -0700, Junio C Hamano wrote:\n>> Can table_size at this point be smaller than 24, which will allow\n>> (table_size - 24) to be a huge number that st_mult() will\n>> comfortably fit?\n>\n> It could be smaller than 24, but I think we're at the point of\n> diminishing returns here.\n\nI only meant to say that we could easily rewrite\n\n\tif (st_mult() > table_size - 24)\n\ncondition to\n\n\tif (st_add(st_mult(), 24) > table_size)\n\nand we do not have to think if we have already checked table_size\nbefore we reach this point of the control flow.\n"},{"id":"496570","messageId":"ZmI6SqIFkWz8OPr1@nand.local","threadId":"61600","inReplyTo":"xmqqjzj1k8c8.fsf@gitster.g","subject":"Re: [PATCH 2/2] pack-bitmap.c: ensure pseudo-merge offset reads are bounded","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-06T22:38:02Z","receivedAt":"2024-06-06T22:38:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jun 06, 2024 at 03:35:51PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > On Thu, Jun 06, 2024 at 12:42:59PM -0700, Junio C Hamano wrote:\n> >> Can table_size at this point be smaller than 24, which will allow\n> >> (table_size - 24) to be a huge number that st_mult() will\n> >> comfortably fit?\n> >\n> > It could be smaller than 24, but I think we're at the point of\n> > diminishing returns here.\n>\n> I only meant to say that we could easily rewrite\n>\n> \tif (st_mult() > table_size - 24)\n>\n> condition to\n>\n> \tif (st_add(st_mult(), 24) > table_size)\n>\n> and we do not have to think if we have already checked table_size\n> before we reach this point of the control flow.\n\nAh. Thanks for the clarification. Yes, I think you could do so; I'm\nhappy to send another version if you like.\n\nThanks,\nTaylor\n"},{"id":"497184","messageId":"xmqqa5jnfko6.fsf@gitster.g","threadId":"61600","inReplyTo":"ZmI6SqIFkWz8OPr1@nand.local","subject":"Re: [PATCH 2/2] pack-bitmap.c: ensure pseudo-merge offset reads are bounded","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-14T18:23:21Z","receivedAt":"2024-06-14T18:23:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Thu, Jun 06, 2024 at 03:35:51PM -0700, Junio C Hamano wrote:\n>> Taylor Blau writes:\n>>\n>> > On Thu, Jun 06, 2024 at 12:42:59PM -0700, Junio C Hamano wrote:\n>> >> Can table_size at this point be smaller than 24, which will allow\n>> >> (table_size - 24) to be a huge number that st_mult() will\n>> >> comfortably fit?\n>> >\n>> > It could be smaller than 24, but I think we're at the point of\n>> > diminishing returns here.\n>>\n>> I only meant to say that we could easily rewrite\n>>\n>> \tif (st_mult() > table_size - 24)\n>>\n>> condition to\n>>\n>> \tif (st_add(st_mult(), 24) > table_size)\n>>\n>> and we do not have to think if we have already checked table_size\n>> before we reach this point of the control flow.\n>\n> Ah. Thanks for the clarification. Yes, I think you could do so; I'm\n> happy to send another version if you like.\n\nLet's get this thing unstuck; the other larger topic that this topic\nbuilds upon is stuck for too long.\n"},{"id":"497190","messageId":"cover.1718392943.git.me@ttaylorr.com","threadId":"61600","inReplyTo":"a71ec05e5dc0c8c40e1cce14a7c5fe946437a24d.1717699237.git.me@ttaylorr.com","subject":"[PATCH v2 0/2] pseudo-merge: various small fixes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-14T19:23:52Z","receivedAt":"2024-06-14T19:23:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is a small reroll of a couple of patches I wrote to fix various\nsmall issues with the tb/pseudo-merge-reachability-bitmaps topic.\n\nThe only change since last time is replacing:\n\n if (st_mult(index->pseudo_merges.nr, sizeof(uint64_t)) > table_size - 24)\n\nwith:\n\n if (st_add(st_mult(index->pseudo_merges.nr, sizeof(uint64_t)), 24) > table_size)\n\nbased on helpful review from Junio. For convenience, a range-diff is\nbelow. Thanks in advance for any final review on this topic :-).\n\nTaylor Blau (2):\n Documentation/technical/bitmap-format.txt: add missing position table\n pack-bitmap.c: ensure pseudo-merge offset reads are bounded\n\n Documentation/technical/bitmap-format.txt | 9 +++++++++\n pack-bitmap.c | 5 +++++\n 2 files changed, 14 insertions(+)\n\nRange-diff against v1:\n-: ---------- > 1: a71ec05e5d Documentation/technical/bitmap-format.txt: add missing position table\n1: 0a16399d14 ! 2: 8abd564e7c pack-bitmap.c: ensure pseudo-merge offset reads are bounded\n @@ Commit message\n end of the mmap'd region.\n\n Prevent this by ensuring that we have at least `table_size - 24` many\n - bytes available to read (subtracting 24 as the length of the metadata\n - component).\n + bytes available to read (adding 24 to the left-hand side of our\n + inequality to account for the length of the metadata component).\n\n This is sufficient to prevent us from reading off the end of the\n pseudo-merge extension, and ensures that all of the get_be64() calls\n @@ pack-bitmap.c: static int load_bitmap_header(struct bitmap_index *index)\n \t\t\t\tindex->pseudo_merges.commits_nr = get_be32(index_end - 20);\n \t\t\t\tindex->pseudo_merges.nr = get_be32(index_end - 24);\n\n -+\t\t\t\tif (st_mult(index->pseudo_merges.nr, sizeof(uint64_t)) > table_size - 24)\n ++\t\t\t\tif (st_add(st_mult(index->pseudo_merges.nr,\n ++\t\t\t\t\t\t sizeof(uint64_t)),\n ++\t\t\t\t\t 24) > table_size)\n +\t\t\t\t\treturn error(_(\"corrupted bitmap index file, pseudo-merge table too short\"));\n +\n \t\t\t\tCALLOC_ARRAY(index->pseudo_merges.v,\n\nbase-commit: 0b7500dc66ffcb6b1ccc3332715936a59c6b5ce4\n--\n2.45.0.33.g0a16399d14.dirty\n"},{"id":"497191","messageId":"a71ec05e5dc0c8c40e1cce14a7c5fe946437a24d.1718392943.git.me@ttaylorr.com","threadId":"61600","inReplyTo":"cover.1718392943.git.me@ttaylorr.com","subject":"[PATCH v2 1/2] Documentation/technical/bitmap-format.txt: add missing position table","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-14T19:23:55Z","receivedAt":"2024-06-14T19:23:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"While investigating a benign Coverity warning on the new pseudo-merge\nimplementation, I was struggling to understand the (paraphrased) below:\n\n ofs = index_end - 24 - (index->pseudo_merges.nr * sizeof(uint64_t));\n for (i = 0; i < index->pseudo_merges.nr; i++) {\n index->pseudo_merges.v[i].at = get_be64(ofs);\n ofs += sizeof(uint64_t);\n }\n\n, in pack-bitmap.c::load_bitmap_header(). Looking at the documentation,\nthe diagram describing the on-disk format (prior to this patch)\nsuggested that the optional extended lookup table immediately preceded\nthe trailing metadata portion.\n\nIf that were the case, that would make the above code from\nload_bitmap_header() incorrect, as we'd be blindly reading into the\nextended offset table.\n\nBut later on in the documentation there is a description of the\npseudo-merge position table as immediately preceding the trailing\nmetadata portion of the extension. And indeed, we do write the position\ntable in pack-bitmap-write.c:\n\n /* write positions for all pseudo merges */\n for (i = 0; i < writer->pseudo_merges_nr; i++)\n hashwrite_be64(f, pseudo_merge_ofs[i]);\n\n hashwrite_be32(f, writer->pseudo_merges_nr);\n hashwrite_be32(f, kh_size(writer->pseudo_merge_commits));\n hashwrite_be64(f, table_start - start);\n hashwrite_be64(f, hashfile_total(f) - start + sizeof(uint64_t));\n\nSo this is purely a case of the diagram being out of sync with the\ntextual description and actual implementation of the format\nspecification.\n\nAdd the missing component back to the format diagram to avoid further\nconfusion in this area.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/technical/bitmap-format.txt | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/Documentation/technical/bitmap-format.txt b/Documentation/technical/bitmap-format.txt\nindex ee7775a258..bfb0ec7beb 100644\n--- a/Documentation/technical/bitmap-format.txt\n+++ b/Documentation/technical/bitmap-format.txt\n@@ -312,6 +312,15 @@ the end of a `.bitmap` file. The format is as follows:\n | |\n +-------------------------------------------+\n | |\n+| Pseudo-merge position table |\n+| +----+----------+----------+----------+ |\n+| | N | Offset 1 | .... | Offset N | |\n+| +----+----------+----------+----------+ |\n+| | | 8 bytes | .... | 8 bytes | |\n+| +----+----------+----------+----------+ |\n+| |\n++-------------------------------------------+\n+| |\n | Pseudo-merge Metadata |\n | +-----------------------------------+ |\n | | # pseudo-merges (4 bytes) | |\n-- \n2.45.0.33.g0a16399d14.dirty\n\n"},{"id":"497192","messageId":"8abd564e7cbea8a3a165db86354d10a3f966d50f.1718392943.git.me@ttaylorr.com","threadId":"61600","inReplyTo":"cover.1718392943.git.me@ttaylorr.com","subject":"[PATCH v2 2/2] pack-bitmap.c: ensure pseudo-merge offset reads are bounded","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-14T19:23:58Z","receivedAt":"2024-06-14T19:24:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"After reading the pseudo-merge extension's metadata table, we allocate\nan array to store information about each pseudo-merge, including its\nbyte offset within the .bitmap file itself.\n\nThis is done like so:\n\n pseudo_merge_ofs = index_end - 24 -\n (index->pseudo_merges.nr * sizeof(uint64_t));\n for (i = 0; i < index->pseudo_merges.nr; i++) {\n index->pseudo_merges.v[i].at = get_be64(pseudo_merge_ofs);\n pseudo_merge_ofs += sizeof(uint64_t);\n }\n\nBut if the pseudo-merge table is corrupt, we'll keep calling get_be64()\npast the end of the pseudo-merge extension, potentially reading off the\nend of the mmap'd region.\n\nPrevent this by ensuring that we have at least `table_size - 24` many\nbytes available to read (adding 24 to the left-hand side of our\ninequality to account for the length of the metadata component).\n\nThis is sufficient to prevent us from reading off the end of the\npseudo-merge extension, and ensures that all of the get_be64() calls\nbelow are in bounds.\n\nHelped-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n pack-bitmap.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 70230e2647..ae13d7ee3b 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -238,6 +238,11 @@ static int load_bitmap_header(struct bitmap_index *index)\n \t\t\t\tindex->pseudo_merges.commits_nr = get_be32(index_end - 20);\n \t\t\t\tindex->pseudo_merges.nr = get_be32(index_end - 24);\n \n+\t\t\t\tif (st_add(st_mult(index->pseudo_merges.nr,\n+\t\t\t\t\t\t sizeof(uint64_t)),\n+\t\t\t\t\t 24) > table_size)\n+\t\t\t\t\treturn error(_(\"corrupted bitmap index file, pseudo-merge table too short\"));\n+\n \t\t\t\tCALLOC_ARRAY(index->pseudo_merges.v,\n \t\t\t\t\t index->pseudo_merges.nr);\n \n-- \n2.45.0.33.g0a16399d14.dirty\n"},{"id":"497199","messageId":"CABPp-BF71VWMmSHNx7-dbs8ek+dK0kPTUnSv2OJmM9VYq-LOkQ@mail.gmail.com","threadId":"61600","inReplyTo":"cover.1718392943.git.me@ttaylorr.com","subject":"Re: [PATCH v2 0/2] pseudo-merge: various small fixes","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-06-14T21:08:53Z","receivedAt":"2024-06-14T21:09:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jun 14, 2024 at 7:23 PM Taylor Blau wrote:\n>\n> Here is a small reroll of a couple of patches I wrote to fix various\n> small issues with the tb/pseudo-merge-reachability-bitmaps topic.\n>\n> The only change since last time is replacing:\n>\n> if (st_mult(index->pseudo_merges.nr, sizeof(uint64_t)) > table_size - 24)\n>\n> with:\n>\n> if (st_add(st_mult(index->pseudo_merges.nr, sizeof(uint64_t)), 24) > table_size)\n>\n> based on helpful review from Junio. For convenience, a range-diff is\n> below. Thanks in advance for any final review on this topic :-).\n>\n> Taylor Blau (2):\n> Documentation/technical/bitmap-format.txt: add missing position table\n> pack-bitmap.c: ensure pseudo-merge offset reads are bounded\n>\n> Documentation/technical/bitmap-format.txt | 9 +++++++++\n> pack-bitmap.c | 5 +++++\n> 2 files changed, 14 insertions(+)\n>\n> Range-diff against v1:\n> -: ---------- > 1: a71ec05e5d Documentation/technical/bitmap-format.txt: add missing position table\n> 1: 0a16399d14 ! 2: 8abd564e7c pack-bitmap.c: ensure pseudo-merge offset reads are bounded\n> @@ Commit message\n> end of the mmap'd region.\n>\n> Prevent this by ensuring that we have at least `table_size - 24` many\n> - bytes available to read (subtracting 24 as the length of the metadata\n> - component).\n> + bytes available to read (adding 24 to the left-hand side of our\n> + inequality to account for the length of the metadata component).\n>\n> This is sufficient to prevent us from reading off the end of the\n> pseudo-merge extension, and ensures that all of the get_be64() calls\n> @@ pack-bitmap.c: static int load_bitmap_header(struct bitmap_index *index)\n> index->pseudo_merges.commits_nr = get_be32(index_end - 20);\n> index->pseudo_merges.nr = get_be32(index_end - 24);\n>\n> -+ if (st_mult(index->pseudo_merges.nr, sizeof(uint64_t)) > table_size - 24)\n> ++ if (st_add(st_mult(index->pseudo_merges.nr,\n> ++ sizeof(uint64_t)),\n> ++ 24) > table_size)\n> + return error(_(\"corrupted bitmap index file, pseudo-merge table too short\"));\n> +\n> CALLOC_ARRAY(index->pseudo_merges.v,\n>\n> base-commit: 0b7500dc66ffcb6b1ccc3332715936a59c6b5ce4\n> --\n> 2.45.0.33.g0a16399d14.dirty\n\nSeries looks good to me.\n"},{"id":"497201","messageId":"xmqq1q4zdxrb.fsf@gitster.g","threadId":"61600","inReplyTo":"CABPp-BF71VWMmSHNx7-dbs8ek+dK0kPTUnSv2OJmM9VYq-LOkQ@mail.gmail.com","subject":"Re: [PATCH v2 0/2] pseudo-merge: various small fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-14T21:23:36Z","receivedAt":"2024-06-14T21:23:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> On Fri, Jun 14, 2024 at 7:23 PM Taylor Blau wrote:\n>>\n>> Here is a small reroll of a couple of patches I wrote to fix various\n>> small issues with the tb/pseudo-merge-reachability-bitmaps topic.\n>>\n> ...\n> Series looks good to me.\n\nThanks, it looked good to me, too. Let's roll it into the base\ntopic that has been waiting in 'next'.\n\n"}]}