Make user/group lookup caching thread-safe #2843
Conversation
pmatilai
force-pushed
the
rpmug-safe
branch
2 times, most recently
from
657079d
to
c320341
Compare
|
(commit message tweaked somewhat in the later pushes) |
|
I wonder if this leaks memory. We create a new struct for each thread. Yes, this is freed in rpmChrootSet but that is relying a lot on the right call order. Edit: Also it ends itself right away if there is no chroot. |
|
It does leak, should've mentioned that in the commit message. In the sense that each thread calling it will leave one cache around and reachable. It's far from ideal but it's basically an emergency bandaid. What do you mean about ending itself? |
|
OK, "ending itself" is a bit over dramatic... rpmChrootSet instantly returns if there is now chroot so rpmugFree() is never called at all. |
|
Yup, but this patch doesn't change that, the "leak" is already there. This only makes it per-thread. |
|
It wouldn't be hard to introduce per-thread locking instead. The bigger deal here is the new central struct that makes it possible to do stuff, perhaps I should actually split that into a separate commit and then consider the thread safety separately. The reason its lumped into one is basically wanting a single patch to fix the issue and people to cherry-pick, but that's clearly bending other "rules" here. |
|
Oh, except that rpmugGname() and rpmugUname() additionally rely on central storage of the returned values so simple mutex locking doesn't work for those. So those would have to be changed to return malloced data for a simple fix (it wont break any users because there aren't any, at the moment). |
I think this was here previously but has gotten erreonously removed at some point - we do use rpmug from librpm so we need to free it too.
Collect all the cached data into a struct for sanity. It's worth noting that this also simplifies the caching, we no longer keep separate name to id and id to name caches (totaling four). We simply cache one id/name for for user and another for group data. Also, reset ids to 0 rather than -1, this is far more obviously a safe value as we have special cases to handle id 0. rpmugUname() and rpmugGname() are have no users in the current codebase, so this was developed wrt commit c576d96 where they still are used, to avoid nasty surprises later on if somebody finds a new case for these.
Now with the caching data in a single struct from the previous commit, we can easily make this per-thread for a minimal fix for thread-safety. Fixes: rpm-software-management#2826
|
Split to refactor + thread-safe fix, and restored that rpmugFree() on librpm exit that had gone missing at some point. |
|
Looks good to me. |
|
It seems like the commit hash mentioned in the commit message isn't correct (it's the "Bump CI" commit which doesn't seem to have anything to do with this). |
It's actually explained in the message:
Ie, that commit is the one before the one that removed all rpmugUname() and rpmugGname() users, so I had to develop against that to be sure this rework actually works. |
This seems like a huge overkill when in 4.19.1 there's exactly one rpmfiStat() call that unnecessarily invokes an rpmug lookup in a threaded scenario, but then rpmfiStat() and rpmfilesStat() are public APIs that people expect to be safe for use within the originating thread.
Collect all the caching data into a struct and allocate on per-thread basis, this seems like the path of least trouble in this case and git diff --stat agrees.
It's worth noting that this also simplifies the caching, we no longer keep separate name to id and id to name, totaling caches totaling four. We simply cache one id/name for for user and another for group data. Also, reset ids to 0 rather than -1, this is far more obviously a safe value as we have special cases to handle id 0.
rpmugUname() and rpmugGname() are have no users in the current codebase, so this was developed wrt commit c576d96 where they still are used, to avoid nasty surprises later on if somebody finds a new case for these.
Fixes: #2826