[Rpm-maint] [rpm-software-management/rpm] 3 cleanup fixes (#359)
You can view, comment on, or merge this pull request online at: https://github.com/rpm-software-management/rpm/pull/359 -- Commit Summary -- * ndb glue: closeEnv() should always clear rdb->db_dbenv * rpmidxHandleObsolete(): Fix fd leak in error path * Fix leak with openDatabase() / rpmdbClose() * gitignore update -- File Changes -- M .gitignore (2) M lib/backend/ndb/glue.c (2) M lib/backend/ndb/rpmidx.c (6) M lib/rpmdb.c (17) -- Patch Links -- https://github.com/rpm-software-management/rpm/pull/359.patch https://github.com/rpm-software-management/rpm/pull/359.diff -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/359 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] 3 cleanup fixes (#359)
FYI: I had to revert the ndb glue change, as it caused segfaults if just an index dbi got closed. The commit doesn't contain any information about the problem it tries to solve, do you remember why your change was necessary? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/359#issuecomment-574609659___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] 3 cleanup fixes (#359)
Hmm, the ndb glue change here looks decidedly wrong to me, the thing is reference counted for a reason (guess this shows just how much attention I've been paying to ndb changes, and I guess that needs to change from now on) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/359#issuecomment-574616568___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] 3 cleanup fixes (#359)
Bad reading, bad patch. Sorry for the NULL pointer dereference. I'm not sure, I might have mixed up somewhere with the different backends. That said, the real problem could have been confusion about the code that calls closeEnv(). It doesn't seem to make sense to test if `rdb->db_dbenv` is NULL before calling closeEnv(). It suggests we're not sure if we've called openEnv() or not. But if we're not sure, then how do we know whether to decrement `rdb->db_dbenv->refs` or not? IOW, I think what I should have sent is: ```diff diff --git a/lib/backend/ndb/glue.c b/lib/backend/ndb/glue.c index 90c10f889..46e115846 100644 --- a/lib/backend/ndb/glue.c +++ b/lib/backend/ndb/glue.c @@ -74,8 +74,7 @@ static int ndb_Close(dbiIndex dbi, unsigned int flags) rpmidxClose(dbi->dbi_db); rpmlog(RPMLOG_DEBUG, "closed db index %s\n", dbi->dbi_file); } -if (rdb->db_dbenv) - closeEnv(rdb); +closeEnv(rdb); dbi->dbi_db = 0; return 0; } ``` -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/359#issuecomment-574760188___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] 3 cleanup fixes (#359)
Seems okay, do a rebase for all checks to pass, hopefully this should be sufficient for it to be merged. @pmatilai , anything else holding this one off? 👍 -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/359#issuecomment-366882575___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] 3 cleanup fixes (#359)
Thanks for review. I have blindly rebased and pushed this; there were no conflicts. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/359#issuecomment-366953925___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] 3 cleanup fixes (#359)
The holdup here has been the rpmdbClose() changes, I've gotten rather wary of changes to that area due to recent fix-regression-fix-regression round - see commit 4c6e51e2c0e3deeb052ae3c47115b6d10cb0d696 which your change almost reverts, and then the subsequent fixes and reverts in the history. We need to be sure it doesn't reintroduce the double-free (which doesn't occur with rpm itself, only in a specific API usage) which means I (or somebody) needs to chase down the reproducer for the original problem etc. In the meanwhile I merged the other changes, those were obvious enough. And FWIW, this is a good example of why combining unrelated changes into a single PR is not necessarily a good idea. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/359#issuecomment-368809438___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] 3 cleanup fixes (#359)
Eh. It can cause a bit of awkwardness or need for discussion when you get unlucky, agreed. So far I'm not very keen to send 3+ different PRs for small fixes to cleanup when they don't depend on each other. I appreciate the attention to detail. Cleanup fixes can be fiddly to get right even if there isn't a concern with the API. Perhaps one argument for a topic PR that gets you in the right mood for nitpicking :). What a fun commit "avoid double free ... assume that the object has been freed if it is not on the list." Good luck :). -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/359#issuecomment-368819363___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] 3 cleanup fixes (#359)
Closed #359. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/359#event-1661156271___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] 3 cleanup fixes (#359)
Finally got around to investigate the rpmdb leak thing properly... @ffesti pointed out that perhaps a better fix is to ensure the new handle is always added to the list. And indeed, this actually fixes another problem too: there was a window where a newly opened database would not be properly closed on arriving signal. Thus, fixed somewhat differently in commit fab7348d9a338349e5727f87af734c0e20cfb7ad. And with that, we're done here. Thanks for the patches and the patience :) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/359#issuecomment-394336365___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] 3 cleanup fixes (#359)
Agreed, fab7348 removes the need for my commit. I notice exit() does not seem to be on the list of POSIX signal-safe functions (in contrast with `_exit()`). I guess in general, if rpm is really trying to clean up inside a signal handler, I'd expect a lot of really hairy edge cases. E.g. technically I think librpm might also be relying on `_free()` not being inlined (LTO). Otherwise, I think the compiler might have the opportunity to re-order freeing the db _before_ updating `*prev`. Leading to double-free if a signal hits at that point. https://github.com/rpm-software-management/rpm/blob/fab7348d9a338349e5727f87af734c0e20cfb7ad/lib/rpmdb.c#L421 Thank you for working on rpm. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/359#issuecomment-394375192___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] 3 cleanup fixes (#359)
Oh, rpm doesn't do clean-up from signal handlers, that's specifically documented as being unsafe for BDB in particular. Rpm has a "signal queue" which is polled from here and there in the codepaths and that's where cleanup occurs. The race here is that the signal queue for trapping those signals for safe handling was not set up, so rpm would directly exit on signals in that window, and this makes BDB rather unhappy. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/359#issuecomment-394621697___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] 3 cleanup fixes (#359)
Got it. I jumped to the wrong conclusion seeing sigaction() being used together with `rpmsigTbl`. I missed that that particular line was saving the old signal disposition, not setting the new one. That definitely makes more sense for the name "signal queue" :-). -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/359#issuecomment-394629170___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
