-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix a segfault in the git fetcher #6582
Conversation
The git fetcher code used to dereference the (potentially empty) `ref` input attribute. This was magically working, probably because the compiler somehow outsmarted us, but is now blowing up with newer nixpkgs versions. Fix that by not trying to access this field while we don't know for sure that it has been defined. Fix #6554
why don't we move this part above the "is-local" check? the code duplication seems unnecessary, especially because the if (!input.getRef()) {
auto head = isLocal ? readHead(actualUrl) : readHeadCached(actualUrl);
if (!head) {
warn("could not read HEAD ref from repo at '%s', using 'master'", actualUrl);
head = "master";
}
input.attrs.insert_or_assign("ref", *head);
unlockedAttrs.insert_or_assign("ref", *head);
} Also, what's going on with the following (although probably unrelated to this PR): Line 445 in ec07a70
The following also doesn't really make any sense... (when getRef() returned nullopt or such, we explicitly dereference it afterwards, instantly UB)Lines 471 to 473 in ec07a70
the meaning/usage of Line 518 in ec07a70
|
I ran into this issue with nix eval --expr 'builtins.fetchGit { url = "https://github.com/mozilla/nixpkgs-mozilla.git"; rev = "15b7a05f20aab51c4ffbefddb1b448e862dccb7d"; }' |
I guess this should be merged because the breakage on nixpkgs master/unstable of nixUnstable is extremely annoying... we can improve the code afterwards if necessary; it appears good enough for now, the more problematic parts of git fetcher are pretty much unrelated to this. |
Yup' I agree with @zseri, #6530 doesn't look like it can be merged in a matter of days, so it seems important to have this fix land since it's pretty bad |
I can't reproduce this on current master (2.10.0pre20220531_04a699b).
|
Yes, as stated in the issue, this only shows for newer nixpkgs versions. |
Successfully created backport PR #6597 for |
The git fetcher code used to dereference the (potentially empty)
ref
input attribute. This was magically working, probably because the
compiler somehow outsmarted us, but is now blowing up with newer nixpkgs
versions.
Fix that by not trying to access this field while we don't know for sure
that it has been defined.
Fix #6554
@harrisonthorne if you have a way to reproduce the original issue, can you confirm that the fix is correct?
Related to #6509