Hi Patrick
On 29/04/2024 14:41, Patrick Steinhardt wrote:
The `is_pseudoref()` function has somewhat weird behaviour in that it
both checks whether a reference looks like a pseudoref, but also that
the reference actually resolves to an object ID.
In case a reference does not resolve though we can run into a segfault
because we never initialize the local `struct object_id` variable. Thus,
when `refs_resolve_ref_unsafe()` is unable to resolve the reference, the
variable will stay uninitialize. We then try to look up the hash algo
s/uninitialize/uninitialized/
via the uninitialized value when calling `is_null_oid()`, which causes
us to segfault.
It is somewhat questionable in the first place that we declare a ref to
be a pseudorefe depending on whether it resolves to an object ID or not.
If I remember rightly Karthik added that check to avoid the files
backend calling a file with a name that matched the pseudoref syntax a
pseudoref when it wasn't actually a pseudoref.
And to make things even worse, a symbolic ref is currently considered to
not be a pseudo ref either because of `RRESOLVE_REF_NO_RECURSE`,
s/RR/R/
That was a deliberate choice to fit with the definition of pseudorefs
excluding symbolic refs.
which
will cause us to not resolve them to an object ID. Last but not least,
it also is inconsistent with `is_headref()`, which only checks for the
reference to exist via `refs_ref_exists()`.
Refactor the code to do the same. While that still feels somewhat fishy,
it at least fixes the segfault for now.
Alternatively we could call oidclr() when refs_resolve_refs_unsafe()
returns NULL
Best Wishes
Phillip
I have not been able to come up
with a reproducible test case that does not rely on other bugs and very
intricate state.
Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
---
refs.c | 17 ++++-------------
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/refs.c b/refs.c
index 567c6fc6ff..b35485f150 100644
--- a/refs.c
+++ b/refs.c
@@ -900,7 +900,6 @@ int is_pseudoref(struct ref_store *refs, const char *refname)
"NOTES_MERGE_REF",
"MERGE_AUTOSTASH",
};
- struct object_id oid;
size_t i;
if (!is_pseudoref_syntax(refname))
@@ -908,20 +907,12 @@ int is_pseudoref(struct ref_store *refs, const char *refname)
if (is_special_ref(refname))
return 0;
- if (ends_with(refname, "_HEAD")) {
- refs_resolve_ref_unsafe(refs, refname,
- RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
- &oid, NULL);
- return !is_null_oid(&oid);
- }
+ if (ends_with(refname, "_HEAD"))
+ return refs_ref_exists(refs, refname);
for (i = 0; i < ARRAY_SIZE(irregular_pseudorefs); i++)
- if (!strcmp(refname, irregular_pseudorefs[i])) {
- refs_resolve_ref_unsafe(refs, refname,
- RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
- &oid, NULL);
- return !is_null_oid(&oid);
- }
+ if (!strcmp(refname, irregular_pseudorefs[i]))
+ return refs_ref_exists(refs, refname);
return 0;
}
