[pacman-dev] memleak caused by "don't duplicate package in cache"
Previously, all installed packages were duplicated when added to the cache. in libalpm/add.c : 801 >-if(_alpm_db_add_pkgincache(db, newpkg) == -1) { And the _alpm_db_add_pkgincache function in cache.c duplicated the newpkg. I wanted to avoid this duplication with the following commit : http://projects.archlinux.org/git/?p=pacman.git;a=commit;h=8240da6cb3ff95ad4... I simply removed the pkg_dup in that function. Since I removed a pkg_dup, I had to remove the corresponding pkg_free call also. Previously, the packages in the targets list were freed in the _alpm_trans_free function. And the duplicated package in the cache were freed in _alpm_db_free_pkgcache. Since the cache is freed only when alpm is released, which happens at the very end, I let the free there. So I removed it from _alpm_trans_free, which is called when a transaction is released. Now the ugly part begins. The targets are only added to the cache when they are installed, not when they are removed (obviously). That explains the condition I added in trans_free : + } else if (trans->type == PM_TRANS_TYPE_REMOVE || + trans->type == PM_TRANS_TYPE_REMOVEUPGRADE) { alpm_list_free_inner(trans->packages, (alpm_list_fn_free)_alpm_pkg_free); So, when a whole install transaction goes fine, without errors, everything works well. All targets are added to the cache, and are then all freed correctly by the _alpm_db_free_pkgcache function. However, when the transaction doesn't go correctly, the targets are loaded in memory, but they are not installed, and so not freed! All targets stay in memory, and cause a big memleak. For example, a conflict could be detected, and the transaction is aborted (as in sync400). But it might even be possible that a transaction gets aborted in the middle. Maybe because it ran out of memory, or because the user interrupted it, or whatever. So, for solving this issue, maybe we would need a way to track down which packages were loaded in the cache. That should allow us to free everything correctly, which doesn't seem possible currently. We could then free in the trans_free function all targets that are not in the pkg cache. And the remaining targets that are in the pkg cache will be freed when the cache is freed.
On Feb 6, 2008 9:07 AM, Xavier
So, for solving this issue, maybe we would need a way to track down which packages were loaded in the cache. That should allow us to free everything correctly, which doesn't seem possible currently. We could then free in the trans_free function all targets that are not in the pkg cache. And the remaining targets that are in the pkg cache will be freed when the cache is freed.
I'm still a fan of handling this sort of thing internally to the list structure, so we gain something like this: struct alpm_list_head { ... ... void (*free_func)(void*); ... }; then the lists which require free-ing of the internal members have this set and it is called on alpm_list_free() is non-NULL. Say, for instance, the cache lists get a free_func = list_free_pkg, all the packages would get free'd when the list is free'd This way we could cover this case here, when we hit the case that the packages need freeing, we simply set the free_func to be non-NULL and that base is covered.
Aaron Griffin wrote:
I'm still a fan of handling this sort of thing internally to the list structure, so we gain something like this:
struct alpm_list_head { ... ... void (*free_func)(void*); ... };
then the lists which require free-ing of the internal members have this set and it is called on alpm_list_free() is non-NULL. Say, for instance, the cache lists get a free_func = list_free_pkg, all the packages would get free'd when the list is free'd
This way we could cover this case here, when we hit the case that the packages need freeing, we simply set the free_func to be non-NULL and that base is covered.
But, here we have two different lists, cache_list and target_list which share the same packages. In the normal case, we have cache_list == target_list (they have the same size, and share the same packages, but not the same nodes). But an error can happen at any step. So we could have an empty cache_list, or only one package in it, or two, etc..
participants (2)
-
Aaron Griffin
-
Xavier