Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 1923820 - libdnf crashes microdnf when manipulating the sack from the API via txnupd plugin
Summary: libdnf crashes microdnf when manipulating the sack from the API via txnupd pl...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: libdnf
Version: 34
Hardware: Unspecified
OS: Unspecified
unspecified
high
Target Milestone: ---
Assignee: Jaroslav Rohel
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: dnf-community
TreeView+ depends on / blocked
 
Reported: 2021-02-02 01:39 UTC by Neal Gompa
Modified: 2021-06-15 07:21 UTC (History)
7 users (show)

Fixed In Version: libdnf-0.63.1-1.fc35
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2021-06-15 07:21:19 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)
Screenshot showing crash backtrace when trying to run microdnf (deleted)
2021-02-02 01:39 UTC, Neal Gompa
no flags Details
Screenshot showing crash backtrace running microdnf after commit revert (deleted)
2021-02-02 02:43 UTC, Neal Gompa
no flags Details
Screenshot showing crash backtrace running microdnf after all sack code is reverted (deleted)
2021-02-02 03:43 UTC, Neal Gompa
no flags Details

Description Neal Gompa 2021-02-02 01:39:27 UTC
Created attachment 1754149 [details]
Screenshot showing crash backtrace when trying to run microdnf

Description of problem:
libdnf crashes microdnf when the txnupd plugin attempts to disable installonly packages via the API by using dnf_sack_set_installonly_limit() to set the installonly_limit to 1.

Version-Release number of selected component (if applicable):
0.55.2-2.1 (openSUSE MicroOS 20200131)

How reproducible:
Always

Steps to Reproduce:
The following steps should give you the same failure on Fedora 33+.
Please do not do this on something you care about!

1. dnf copr enable ngompa/transactional-update
2. dnf install microdnf libdnf-plugin-txnupd
3. microdnf --version

Actual results:
Segmentation fault

Expected results:
Print version string

Additional info:
Commit in the txnupd plugin adding the usage of the API: https://code.opensuse.org/microos/libdnf-plugin-txnupd/c/30affd5bdf533f5df700ca7791014d93aabac2e7

Comment 1 Neal Gompa 2021-02-02 02:43:55 UTC
Created attachment 1754153 [details]
Screenshot showing crash backtrace running microdnf after commit revert

Reverting that commit reveals that something is broken about acquiring the sack in a plugin in the first place.

That code in the txnupd plugin was added with this commit: https://code.opensuse.org/microos/libdnf-plugin-txnupd/c/2f7a284a16699ceabd4e1a04448f4a2ff0b82840

Comment 2 Neal Gompa 2021-02-02 03:43:52 UTC
Created attachment 1754160 [details]
Screenshot showing crash backtrace running microdnf after all sack code is reverted

Reverting *that* commit leads to the plugin working, though it seems the SWDB stuff chokes. I'm not sure what is causing that, but the actual behavior is correct, including generating and setting the new snapshot to boot from with the new package installed.

Comment 3 Jaroslav Rohel 2021-02-02 09:35:15 UTC
@Neal Gompa
The main problem is, that it is too early to use sack in the hook PLUGIN_HOOK_ID_CONTEXT_PRE_REPOS_RELOAD. Sack is not initialised.
In addition, the situation is more complicate with PackageKit. If you plan to use your plugin with PackageKit, remember that it is a daemon. I'm not sure now, but I think that PackageKit creates more sacks and goals. And PackageKit calls "dnf_sack_set_installonly_limit" function and can overide your setting.

Solution for "installonly_limit":
We can try to change the main configuration. Probably you do not want to change "dnf.conf" file (line "installonly_limit=1").
There is function "dnf_context_get_installonly_limit" in C libdnf API. But it is not ideal and there is no setter.
What now? There is a general C API for accesing configuration options. I added it to libdnf version 0.56.0 https://github.com/rpm-software-management/libdnf/pull/1109 .
So let's try replacing the call "dnf_sack_set_installonly_limit(dnfSack, 1);" by calling "dnf_conf_main_set_option("installonly_linit", DNF_CONF_PLUGINDEFAULT, "1", NULL);" probably in the PLUGIN_HOOK_ID_CONTEXT_CONF hook.

Your second problam (adding protected packages) is more complicated. I must think about it.

Comment 4 Jaroslav Rohel 2021-02-02 21:43:16 UTC
@Neal Gompa
The second problem - add protected packages to the goal (in general, you want to modify the goal).
The main problem is that it is too early to use the sack in the hook PLUGIN_HOOK_ID_CONTEXT_PRE_REPOS_RELOAD. The sack is not initialized. And too late to change the goal in the hook PLUGIN_HOOK_ID_CONTEXT_PRE_TRANSACTION. At that time, the goal is resolved and the transaction is prepared and tested.

The best solution is to add another hook to libdnf. PLUGIN_HOOK_ID_CONTEXT_PRE_GOAL_SOLVE, which will be called just before the goal is resolved - the place where the plugin would have a chance to change the goal.
And maybe another PLUGIN_HOOK_ID_CONTEXT_GOAL_SOLVED, which will be called shortly after the goal is resolved.

I will look at it. I have a tip on where to add a hook. Some time ago I prepared a pull request to support the "protected_packages" option in the context part of libdnf. https://github.com/rpm-software-management/libdnf/pull/1104 . We agreed in the team that this is a step to be more compatible with DNF. However, we are concerned that someone depends on the old behaviour - ignoring protected_packages in the context part of libdnf. That is why PR was not merged.

Comment 5 Neal Gompa 2021-02-03 03:06:05 UTC
I've submitted an update in openSUSE Factory/Tumbleweed for libdnf 0.58.0 so I can use the suggested API: https://build.opensuse.org/request/show/868790

Comment 6 Neal Gompa 2021-02-03 03:08:56 UTC
(In reply to Jaroslav Rohel from comment #4)
>
> I will look at it. I have a tip on where to add a hook. Some time ago I
> prepared a pull request to support the "protected_packages" option in the
> context part of libdnf.
> https://github.com/rpm-software-management/libdnf/pull/1104 . We agreed in
> the team that this is a step to be more compatible with DNF. However, we are
> concerned that someone depends on the old behaviour - ignoring
> protected_packages in the context part of libdnf. That is why PR was not
> merged.

I don't think it's reasonable to expect that protected packages would be ignored in any part of the stack. Generally, when those are set, it is expected that it is respected, regardless of how the package manager is invoked. If there's a scenario where we need this to be ignored (e.g. microdnf in containers) we should probably configure this accordingly in containers...

Comment 7 Jaroslav Rohel 2021-02-03 08:19:48 UTC
I prepared PR that adds hooks to the "dnf_goal_depsolve" function.
PR https://github.com/rpm-software-management/libdnf/pull/1129

Added hooks:
PLUGIN_HOOK_ID_CONTEXT_PRE_GOAL_SOLVE
Called just before the goal is resolved. The place where the plugin
have a chance to change the goal.

PLUGIN_HOOK_ID_CONTEXT_GOAL_SOLVED
Called shortly after the goal is resolved.

Added functions for accessing DnfPluginHookData delivered to plugins:
HyGoal hookContextGoalGetGoal(DnfPluginHookData * data);
DnfGoalActions * hookContextGoalGetActions(DnfPluginHookData * data);

Warning:
But it's more complicated. New hooks will be called automatically with microdnf. But for example in PackageKit we need to add a line to the code. More information is in PR.

Comment 8 Neal Gompa 2021-02-05 17:59:05 UTC
If you want to see what I've got so far, you can grab test images here: https://download.opensuse.org/repositories/home:/Pharaoh_Atem:/DNF_SUSE:/MicroOS/images/

They require configuration using Ignition (similar to Fedora CoreOS).

Comment 9 Ben Cotton 2021-02-09 16:13:59 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 34 development cycle.
Changing version to 34.

Comment 10 Jaroslav Rohel 2021-02-11 06:50:47 UTC
Update:

installonly_limit:
As mentioned at the community meeting. The setting "installonly_limit = 1" is not supported. Jaroslav Mracek had the idea to empty the "installonlypkgs" list.
So instead of dnf_conf_main_set_option("installonly_linit", DNF_CONF_PLUGINDEFAULT, "1", NULL) call dnf_conf_main_set_option("installonlypkgs", DNF_CONF_PLUGINDEFAULT, "", NULL); It is an "append" option, but writing an empty string will clear it.
Unfortunately, during tests with microdnf, I found that the "installonlypkgs" option is ignored in the "context" part of libdnf. Instead, a hard-coded list of installation packages was always used.
I fixed it in PR https://github.com/rpm-software-management/libdnf/pull/1135.

Adding protected packages:
New hooks to libdnf will not be added now. But my earlier PR was merged https://github.com/rpm-software-management/libdnf/pull/1104.
So you can add name of the protected package to the configuration file (in directory "/etc/dnf/protected.d/"). Or via API. Note that this is not an "append" option. So first load the original list and extend.
Example:
  enum DnfConfPriority prio;
  g_autofree gchar * prot_pkgs = dnf_conf_main_get_option("protected_packages", &prio, NULL);
  g_autofree gchar * new_prot_pkgs = g_strconcat(prot_pkgs, " ", "name_of_protected_package", NULL);
  dnf_conf_main_set_option("protected_packages", DNF_CONF_PLUGINDEFAULT, new_prot_pkgs, NULL);

Comment 11 Jaroslav Rohel 2021-02-11 07:27:57 UTC
PRs were merged in the upstream. I hope that the problems will be solved by the procedure mentioned in comment 10. 
https://github.com/rpm-software-management/libdnf/pull/1104
https://github.com/rpm-software-management/libdnf/pull/1135

Comment 12 Fedora Update System 2021-06-15 07:21:19 UTC
FEDORA-2021-78c5f8c03d has been pushed to the Fedora 35 stable repository.
If problem still persists, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.