Fix leak of dpkg cache when dpkginfo_init is called multiple times #1939
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
So the function is called from pthread_mutex_init (&(g_dpkg.mutex), NULL);So, are you sure it is being called more than once? |
|
I've noticed For example, you can look at this GDB trace: The dpkg probe was instantiated two times during the execution of the _xccdf_policy_rule_evaluate function. |
|
Okay, I can see that the init function is not meant to be executed once. On the contrary, the probe is expected to create its unique context (including mutexes and stuff) every time the init function is called. The static global context of the dpkg probe is simply wrong: struct dpkginfo_global {
int init_done;
pthread_mutex_t mutex;
};
static struct dpkginfo_global g_dpkg = {
.init_done = -1,
};Now, if the @0intro Do you want to try and make a proper fix for the dpkg probe? Because what we have right now is an UB. |
|
Yes. I'll work on a proper fix for the dpkg probe. |
0intro
force-pushed
the
dpkginfo_cache_leak
branch
from
50206b2
to
0e02766
Compare
|
It might be tempting to move the global I think the issue is that I've changed the commit to set What do you think? |
|
We still have a problem with mutex. |
|
Should we add a global mutex on |
0intro
force-pushed
the
dpkginfo_cache_leak
branch
2 times, most recently
from
bfaa758
to
3e9500b
Compare
evgenyz
reviewed
Jun 19, 2023
evgenyz
reviewed
Jun 19, 2023
0intro
force-pushed
the
dpkginfo_cache_leak
branch
2 times, most recently
from
5469d0c
to
fc66249
Compare
|
Thanks for your review. I've recently worked on a better version of this change. The I've made other changes, which are described in the commit message. I think there are still some cleanup which could be done in the |
In the dpkginfo probe, the dpkg cache was allocated in dpkginfo_init. However, dpkginfo_init can be called multiple times, leading the cgCache pointer to be overridden and leaking the dpkg cache. There are usually two dpkginfo probes running at the same time: CPE and OVAL. The opencache function is conditioned by the _init_done flag, however, this flag was never set, so opencache was always called. This change sets _init_done to 1 after opencache has been called, and allocate the dpkg cache only when _init_done is 0. Concurrent access to the cgCache static variable is now protected by a statically initialized mutex in dpkginfo_probe.c. We chose to keep cgCache as a static variable, because allocating cgCache for every thread has big performance impact. Also, calling new/delete revealed memory leaks in libapt. Consequently, cgCache is initialized once and never freed. The call to the cgCache->ReadOnlyOpen method is now done in the dpkginfo_get_by_name function, so the dpkg cache is always up to date. The cgCache->Close method is still called in dpkginfo_ini. This change also removes the unused dpkg_mmap static variable in dpkginfo-helper.cxx.
0intro
force-pushed
the
dpkginfo_cache_leak
branch
from
fc66249
to
f41f0ee
Compare
evgenyz
reviewed
Sep 5, 2023
| dpkginfo_fini(); | ||
| pthread_mutex_destroy (&(d->mutex)); | ||
| pthread_mutex_unlock (&(d->mutex)); |
There was a problem hiding this comment.
Why don't we reset the .init_done here? The global struct is now uninitialized, right?
There was a problem hiding this comment.
The dpkginfo_global structure is initialized statically. .init_done is set to 1 once dpkginfo_init has been called for the first time. dpkginfo_fini only calls cgCache->Close, so cgCache->ReadOnlyOpen could update the dpkg cache the next time dpkginfo_get_by_name will be called. cgCache is allocated once, when dpkginfo_init has been called for the first time. Then, cgCache is never freed.
evgenyz
reviewed
Sep 5, 2023
There was a problem hiding this comment.
I still have my doubts about this global context. What would happen if 2 instances initialize the context and then one of them would finish? How the second remaining instance will operate?
It probably never happens ATM because the instance pool lifetimes are the same, but it could change.
The first call to
Actually, this case happens in our use case (which is different from what the In our use case, we create a session once ( Consequently During the lifetime of the XCCDF session, So, at any point of the lifetime of the XCCDF session, we can have up to two |
evgenyz
approved these changes
Sep 6, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the dpkginfo probe, the dpkg cache was allocated in dpkginfo_init. However, dpkginfo_init can be called multiple times, leading the cgCache pointer to be overridden and leaking the dpkg cache.