From 25e5ceb9ee21d8806c9a3651e4f10241155f6e14 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 2 Apr 2026 00:15:05 -0400 Subject: [PATCH] pseudo-merge: fix disk reads from find_pseudo_merge() The goal of this commit was to fix a const warning when compiling with new versions of glibc, but ended up untangling a much deeper problem. The find_pseudo_merge() function does a bsearch() on the "commits" pointer of a pseudo_merge_map. This pointer ultimately comes from memory mapped from the on-disk bitmap file, and is thus not writable. The "commits" array is correctly marked const, but the result from bsearch() is returned directly as a non-const pseudo_merge_commit struct. Since new versions of glibc annotate bsearch() in a way that detects the implicit loss of const, the compiler now warns. My first instinct was that we should be returning a const struct. That requires apply_pseudo_merges_for_commit() to mark its local pointer as const. But that doesn't work! If the offset field has the high-bit set, we look it up in the extended table via nth_pseudo_merge_ext(). And that function then feeds our const struct to read_pseudo_merge_commit_at(), which writes into it by byte-swapping from the on-disk mmap. But I think this points to a larger problem with find_pseudo_merge(). It is not just that the return value is missing const, but it is missing that byte-swapping! And we know that byte-swapping is needed here, because the comparator we use for bsearch() also calls our read_pseudo_merge_commit_at() helper. So I think the interface is all wrong here. We should not be returning a pointer to a struct which was cast from on-disk data. We should be filling in a caller-provided struct using the bytes we found, byte-swapping the values. That of course raises the dual question: how did this ever work, and does it work now? The answer to the first part is: this code does not seem to be triggered in the test suite at all. If we insert a BUG("foo") call into apply_pseudo_merges_for_commit(), it never triggers. So I think there is something wrong or missing from the test setup, and this bears further investigation. Sadly the answer to the second part ("does it work now") is still "no idea". I _think_ this takes us in a positive direction, but my goal here is mainly to quiet the compiler warning. Further bug-hunting on this experimental feature can be done separately. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- pseudo-merge.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/pseudo-merge.c b/pseudo-merge.c index a2d5bd85f9..ff18b6c364 100644 --- a/pseudo-merge.c +++ b/pseudo-merge.c @@ -638,14 +638,21 @@ static int pseudo_merge_commit_cmp(const void *va, const void *vb) return 0; } -static struct pseudo_merge_commit *find_pseudo_merge(const struct pseudo_merge_map *pm, - uint32_t pos) +static int find_pseudo_merge(const struct pseudo_merge_map *pm, uint32_t pos, + struct pseudo_merge_commit *out) { - if (!pm->commits_nr) - return NULL; + const unsigned char *at; - return bsearch(&pos, pm->commits, pm->commits_nr, - PSEUDO_MERGE_COMMIT_RAWSZ, pseudo_merge_commit_cmp); + if (!pm->commits_nr) + return 0; + + at = bsearch(&pos, pm->commits, pm->commits_nr, + PSEUDO_MERGE_COMMIT_RAWSZ, pseudo_merge_commit_cmp); + if (!at) + return 0; + + read_pseudo_merge_commit_at(out, at); + return 1; } int apply_pseudo_merges_for_commit(const struct pseudo_merge_map *pm, @@ -653,16 +660,15 @@ int apply_pseudo_merges_for_commit(const struct pseudo_merge_map *pm, struct commit *commit, uint32_t commit_pos) { struct pseudo_merge *merge; - struct pseudo_merge_commit *merge_commit; + struct pseudo_merge_commit merge_commit; int ret = 0; - merge_commit = find_pseudo_merge(pm, commit_pos); - if (!merge_commit) + if (!find_pseudo_merge(pm, commit_pos, &merge_commit)) return 0; - if (merge_commit->pseudo_merge_ofs & ((uint64_t)1<<63)) { + if (merge_commit.pseudo_merge_ofs & ((uint64_t)1<<63)) { struct pseudo_merge_commit_ext ext = { 0 }; - off_t ofs = merge_commit->pseudo_merge_ofs & ~((uint64_t)1<<63); + off_t ofs = merge_commit.pseudo_merge_ofs & ~((uint64_t)1<<63); uint32_t i; if (pseudo_merge_ext_at(pm, &ext, ofs) < -1) { @@ -673,11 +679,11 @@ int apply_pseudo_merges_for_commit(const struct pseudo_merge_map *pm, } for (i = 0; i < ext.nr; i++) { - if (nth_pseudo_merge_ext(pm, &ext, merge_commit, i) < 0) + if (nth_pseudo_merge_ext(pm, &ext, &merge_commit, i) < 0) return ret; merge = pseudo_merge_at(pm, &commit->object.oid, - merge_commit->pseudo_merge_ofs); + merge_commit.pseudo_merge_ofs); if (!merge) return ret; @@ -687,7 +693,7 @@ int apply_pseudo_merges_for_commit(const struct pseudo_merge_map *pm, } } else { merge = pseudo_merge_at(pm, &commit->object.oid, - merge_commit->pseudo_merge_ofs); + merge_commit.pseudo_merge_ofs); if (!merge) return ret;