All of lore.kernel.org
 help / color / mirror / Atom feed
From: Patrick Steinhardt <ps@pks.im>
To: git@vger.kernel.org
Cc: Derrick Stolee <derrickstolee@github.com>, Taylor Blau <me@ttaylorr.com>
Subject: [PATCH] commit-graph: fix truncated generation numbers
Date: Mon, 27 Mar 2023 10:08:25 +0200	[thread overview]
Message-ID: <f8a0a869e8b0882f05cac49d78f49ba3553d3c44.1679904401.git.ps@pks.im> (raw)

[-- Attachment #1: Type: text/plain, Size: 2820 bytes --]

In 80c928d947 (commit-graph: simplify compute_generation_numbers(),
2023-03-20), the code to compute generation numbers was simplified to
use the same infrastructure as is used to compute topological levels.
This refactoring introduced a bug where the generation numbers are
truncated when they exceed UINT32_MAX because we explicitly cast the
computed generation number to `uint32_t`. This is not required though:
both the computed value and the field of `struct commit_graph_data` are
of the same type `timestamp_t` already, so casting to `uint32_t` will
cause truncation.

This cast can cause us to miscompute generation data overflows:

    1. Given a commit with no parents and committer date
       `UINT32_MAX + 1`.

    2. We compute its generation number as `UINT32_MAX + 1`, but
       truncate it to `1`.

    3. We calculate the generation offset via `$generation - $date`,
       which is thus `1 - (UINT32_MAX + 1)`. The computation underflows
       and we thus end up with an offset that is bigger than the maximum
       allowed offset.

As a result, we'd be writing generation data overflow information into
the commit-graph that is bogus and ultimately not even required.

Fix this bug by removing the needless cast.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---

This commit applies on top of cbfe360b14 (commit-reach: add
tips_reachable_from_bases(), 2023-03-20), which has recently been merged
to next.

 commit-graph.c                     | 2 +-
 t/t5328-commit-graph-64bit-time.sh | 9 +++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/commit-graph.c b/commit-graph.c
index 172e679db1..b96509354e 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1565,7 +1565,7 @@ static timestamp_t get_generation_from_graph_data(struct commit *c, void *data)
 static void set_generation_v2(struct commit *c, timestamp_t t, void *data)
 {
 	struct commit_graph_data *g = commit_graph_data_at(c);
-	g->generation = (uint32_t)t;
+	g->generation = t;
 }
 
 static void compute_generation_numbers(struct write_commit_graph_context *ctx)
diff --git a/t/t5328-commit-graph-64bit-time.sh b/t/t5328-commit-graph-64bit-time.sh
index 093f0c067a..57e4d9c699 100755
--- a/t/t5328-commit-graph-64bit-time.sh
+++ b/t/t5328-commit-graph-64bit-time.sh
@@ -63,4 +63,13 @@ test_expect_success 'set up and verify repo with generation data overflow chunk'
 
 graph_git_behavior 'overflow 2' repo left right
 
+test_expect_success 'single commit with generation data exceeding UINT32_MAX' '
+	git init repo-uint32-max &&
+	cd repo-uint32-max &&
+	test_commit --date "@4294967297 +0000" 1 &&
+	git commit-graph write --reachable &&
+	graph_read_expect 1 "generation_data" &&
+	git commit-graph verify
+'
+
 test_done
-- 
2.40.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

             reply	other threads:[~2023-03-27  8:08 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-27  8:08 Patrick Steinhardt [this message]
2023-03-28 17:45 ` [PATCH] commit-graph: fix truncated generation numbers Junio C Hamano
2023-03-28 18:32   ` Derrick Stolee
2023-03-28 18:39 ` Taylor Blau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f8a0a869e8b0882f05cac49d78f49ba3553d3c44.1679904401.git.ps@pks.im \
    --to=ps@pks.im \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=me@ttaylorr.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.