Skip to content
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 segfault in repo_internalize_trigger (RhBug:1375895) #596

Merged
merged 1 commit into from Oct 4, 2018

Conversation

m-blaha
Copy link
Member

@m-blaha m-blaha commented Sep 27, 2018

@AdamWill
Copy link
Contributor

this may also fix https://bugzilla.redhat.com/show_bug.cgi?id=1444262 , https://bugzilla.redhat.com/show_bug.cgi?id=1434283 and/or https://bugzilla.redhat.com/show_bug.cgi?id=1480846 ; those are all similar crashes in the same place, but reached via different paths from PackageKit. Whether this fixes them or not depends whether the problem is that repo_internalize_trigger is being called on a libsolv repo that doesn't (no longer?) exists - in which case this should fix them - or that the libsolv repo exists, but the lookup of the associated hawkey repo somehow fails (in which case this would not fix them). I am not sure which problem is happening in which cases.

@m-blaha
Copy link
Member Author

m-blaha commented Sep 27, 2018

@AdamWill I'm trying to find out what happened in the other bugs. Basically looking for possibility, that in dnf exists libsolv repo with appdata (hawkey repo) set to null. But without success so far.

@AdamWill
Copy link
Contributor

yeah, that was the same path I went down, but I just can't find any case - the obvious possibilities are the repo creation bits, where a libsolv repo might get created, then hawkey repo init fail somehow, then the libsolv repo not get properly removed...but I can't see anything like that, at least not unless there's some subtle bug in freeing on the libsolv side. The other obvious possibility would be the hawkey repo somehow getting freed before the libsolv repo did, but I don't see that happening either.

There were two less obvious possibilities I could see. One is the path that runs through dnf_sack_setup_cmdline_repo , which is much sloppier about its setup than the other things. It doesn't use the ref counter, it doesn't check whether hy_repo_create or repo_create actually work. And it does seem to be creating the repo in the sack it's passed, not in its throwaway copy of it. But it doesn't really seem to line up with all the bug reports, which don't seem like they'd be running through that function at all.

The other is PackageKit sack caching. PackageKit actually has this mechanism for caching and reusing sacks. It's implemented in PK's dnf backend, in dnf_utils_create_sack_for_filters:

https://github.com/hughsie/PackageKit/blob/master/backends/dnf/pk-backend-dnf.c#L668-L685

and I think all the crashes I identified run through that. So it's possible that's related. I haven't dug into how those cached sacks could wind up with stale/broken repos, though.

@kalev
Copy link
Collaborator

kalev commented Oct 2, 2018

Re the sack cache in packagekit, I just played a bit with removing the cache and that made gnome-software noticeably slower. I'd rather avoid removing it if possible.

@j-mracek
Copy link
Member

j-mracek commented Oct 2, 2018

I believe that the problem was caused by incored freeing of memory that was fixed by @jrohel, but I cannot find the commit. The code looks fine, therefore we can merge it.

@j-mracek
Copy link
Member

j-mracek commented Oct 2, 2018

@rh-atomic-bot r+

@rh-atomic-bot
Copy link

📌 Commit 81594e9 has been approved by j-mracek

@rh-atomic-bot
Copy link

⌛ Testing commit 81594e9 with merge 390df3f...

@rh-atomic-bot
Copy link

💥 Test timed out

@kalev kalev merged commit 59b3ab0 into rpm-software-management:master Oct 4, 2018
@AdamWill
Copy link
Contributor

AdamWill commented Oct 4, 2018

I believe that the problem was caused by incored freeing of memory that was fixed by @jrohel, but I cannot find the commit. The code looks fine, therefore we can merge it.

It'd be really good if we could find it, as then we could backport that for F27 and F28...

@AdamWill
Copy link
Contributor

AdamWill commented Oct 4, 2018

Was it this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants