[Rpm-maint] Plugin ponderings

Reshetova, Elena elena.reshetova at intel.com
Wed Nov 28 12:40:27 UTC 2012


New version is attached. I have corrected (hopefully) the indentation and
other cosmetics. 
Also, changed the  rpmScriptRun() as you suggested (including bits for
script execution flow indication), it indeed looks much better! 

I also removed the return code assignment from POST hook, because it indeed
overwrites the actual script return code and anyway again there is no way
rpm can handle any failures from this hook. 

And about changing the fsm commit to commit everything after all files are
installed to tmp and all plugins/checks/whatever has executed, I understand
that this would be a BIG change. Just maybe smth for really far future..
Just it could help improve robustness, too IMO. 

Best Regards,

-----Original Message-----
From: Panu Matilainen [mailto:pmatilai at laiskiainen.org] 
Sent: Tuesday, November 27, 2012 12:26 PM
To: Reshetova, Elena
Cc: rpm-maint at lists.rpm.org
Subject: Re: Plugin ponderings

On 11/26/2012 07:26 PM, Reshetova, Elena wrote:
> Hi,
> I am attaching next version of the scriptlet-related patch. Next I 
> will go back to current hooks and make a small patch to make sure they 
> are called as we agreed.

Some issues with the patch still, comments + further ideas below:

In rpmScriptRun(), just grab the internal/extranal bit into a variable early
so you dont need double calls differing only by the internal/external
argument to the hooks. So the whole thing then becomes something like

     int script_type = SCRIPTLET_TYPE_EXTERNAL;

     if (rstreq(args[0], "<lua>")
         script_type = SCRIPTLET_TYPE_INTERNAL;

     rc = rpmpluginsCallScriptletPre(plugins, script->descr,

     if (rc != RPMRC_FAIL) {
         if (script_type == SCRIPTLET_TYPE_INTERNAL)
             rc = runLuaScript(...);
             rc = runExtScript(...);

     rc = rpmpluginsCallScriptletPost(plugins, script->descr,
                                      script_type, rc);

Except that here we get to the return code issues again: the rc returned
from Post hook overrides whatever the earlier result might've been, eg if
the scriptlet execution failed then the post-hook definetely shouldn't turn
that into an ok. That flaw is present both in your patch and my sample code
above. The other question is: if the scriptlet execution succeeded, is there
ever a valid case for a plugin to turn that into a failure? For such a
seemingly simple thing, this is surprisingly tricky to get right :)

BTW perhaps it would make sense to change the internal/external scriptlet
type thing into a bitfield which tells the hooks how exactly the scriptlet
will get executed. Basically an "external" script would be something like
(RPMSCRIPT_FORK|RPMSCRIPT_EXEC) and "internal" scripts neither, until we add
the ability to fork the Lua execution too, at which point Lua scripts get
RPMSCRIPT_FORK (but no EXEC). Something like that should remove any
ambiguosity from what "internal" and "external" 
script actually means.

Then there's a bunch of indentation issues, at least these:

- missing indentation level on all the new rpmpluginsCallScriptletFoo() for
loops, eg:

     for (i = 0; i < plugins->count; i++) {
     name = plugins->names[i];
     if (hookFunc(s_name, type) == RPMRC_FAIL)
         rc = RPMRC_FAIL;

..and here:

         if (xx == 0) {
         xx = execv(argv[0], argv);

...and a few cases of soft tabs where hard tabs should be used. Hard vs soft
tab mixup is mostly just a slight annoyance (due to diffs not aligning
"perfectly") and not the end of the world, but the indentation levels really
need to be right.

Another minor cosmetics issue I seem to have missed on previous rounds: 
the ampersand placement in K&R is next to the variable name, not the type.
Eg "char* s_name" should be "char *s_name" instead (although a lot of rpm
code has whitespace on both sides, thats ok too)

> Will do this tomorrow. Also some comments below:
>> There's eg RPMRC_NOTTRUSTED that could be (ab)used for certain types 
>> of
> security-related failures, but what if we have several plugins on a 
> hook, one returning RPMRC_OK, another one RPMRC_FAILED and the third 
> one RPMRC_NOTTRUSTED - which >one should the CallFooHook() function
>> One of the failures sure but...
> The security theory is very easy for this case: the most restrictive 
> case/result is applied/returned. So, in this case I would return that 
> we failed on policy and indicate it with " RPMRC_NOTTRUSTED ". In this 
> way rpm can always enforce the strictest rule, for example if it wants 
> to abort installation after receiving RPMRC_NOTTRUSTED.

In a purely security context that certainly makes sense, but in our
case(s) the results tend to be mixups of potential security and miscellanous
other errors. At least to me its not at all obvious which should "win" in
various circumstances.

>> One possibility could be a bitfield type return code, where each 
>> plugin
> can raise relevant bits of fairly generic categories. There's no 
> telling which plugin raised what bits, but at least it'd be possible 
> to reflect more than one kind of failure per hook then. >But again, 
> mostly just something to keep in mind / think about for now.
> This would be cool, especially if we get LSM stacking accepted 
> upstream! Now there are active discussions on security mailing list on 
> Casey's patches and I can imagine that in the some distant future we 
> can have a number of simultaneous LSM working with rpm and a number of
plugins that can use it.
> And for each plugin we can log what happens and make rpm to be more aware.
> But you are right: this is just future so far...

Right, perhaps I should look into this a bit. There are numerous places
within rpm that would benefit from bitfield error codes, for example the
rpmtsRun() return code is absolutely useless as it is now.

>> We could just make the hook return "void", but perhaps its better to 
>> let
> the hooks return errors codes and let rpm filter out (and perhaps log 
> a
>> warning) what it cant handle: its possible that some things it can't 
>> handle
> now can be handled at some point in future.
> Agree, I think it is good to give plugins possibility to complain, 
> even if the complains aren't heard at the moment. I just meant that 
> maybe no reason to invent new return codes just yet.

Yup, lets see how far we can get with the rpmRC codes.

>> Note that rpm always creates files (but not directories) with a 
>> temporary
> extension, and they're only rename()'d to final location (which might 
> replace existing files on upgrades) after everything was successfully 
> unpacked to the temporary locations. Rpm >doesn't currently do a 
> proper job of cleaning up the cruft in case of failure, but there is a 
> chance to undo at least some of the things before the fsm "commits"
>> the files to final destination.
> Oh, this is actually very good, but I am not sure this is full 
> solution. I guess I need to study fsm machine more deeply. It is the 
> hardest one for me to understand so far :(

Heh, it quite probably is the most intricate piece of code there is in rpm,
even after several rounds of gradual cleanups over the years. Lets just say
we have no shortage of <cough> colorful <cough> expansions for the FSM

> Let's imagine we install a package and while putting its files to temp.
> location security labelling fails on one of them (maybe even not the 
> first file and this is the bad thing). Plugin returns failure on that 
> file in file closed hook (which in this case would need to be moved before
FSM commit).
> What would be the proper behaviour? IMO we need to revert the whole 
> package installation unfortunately. Because if file is left on 
> filesystem but not properly labelled, then it might not be usable, and 
> this might also lead to app/service/daemon/whateverinstalledfrom 
> package not usable too.  Do we want such package on the filesystem then?

Yup, failure on file creation (including setting its permissions, security
labels etc) should fail the entire package. I dont remember the exact
details of what currently happens in all the possible cases, but it should
be possible to improve the behavior considerably by doing more things before
the "commit" step, and implement proper cleanup in case of failure. OTOH at
this point, the package can already have executed some scripts (eg %pretrans
and %pre) and actions done by those can't be undone :-/

> But then the problem how do we remove
> the files that has already been commited or if this file happens to be 
> a dir?

For "normal" cases, undoing should be reasonably doable. Directories and
directory-symlinks are much much tougher cookies though.

>> At least in SELinux, rename() doesn't affect the security labeling so 
>> it'd
> be possible to set the final contexts on the temporary files and only 
> if that succeeds for all files, commit the result. Currently the 
> labelling is done on the final destination so there's not >much chance 
> of undoing in case of failure.
> Hm, isn't rename just a sub-case of mv? I think  in this case, xattrs 
> might not be preserved (for example for cp if you don't use 
> --preserve=xattrs, then you might lose your xattrs, including security 
> labels (applies for both SELinux and any LSM using the xattrs)). I 
> would need to setup a small test to remember this. But there should be 
> a way for sure to move the files from temporal location to final one 
> while preserving all attributes, given that rpm has needed privileges
(which it does).

It's a sub-case of mv, but rename() is a "pure" rename and requires oldpath
and newpath to be on the same filesystem. Except for time stamp updates it
touches no other metadata - ownership, permissions etc do not chance in
rename(). mv on the other hand can cross mount-points, in which case its
actually a copy (and requires special attention to permissions, xattrs and

> I wonder if rpm could first
> unpack all files and dirs to some temp location and then do one FSM 
> commit (if everything went fine) to commit them all one by one. This 
> would solve the problem of how to remove already installed files.

That's *sort of* what it already does, except that permissions etc are set
as part of commit instead of creation time, and since permissions & labels
etc can fail...

It should be possible to considerably improve the current behavior: the
to-be-replaced files could be moved to another temporary name before
replacing so we could try go back to previous contents if commit fails, and
we could try limiting the amount of things that can fail in commit. 
And we should clean up temporary leftovers in case of failure anyway.
But here too, directories are much tougher cookies.

	- Panu -

>> Yup... although some of this could be perhaps dealt with by 
>> rearranging
> the way file permissions etc are laid down: currently they're all done 
> on the final destination, but if the labels, capabilities, modes etc 
> were staged in it would allow dealing with, well, >not all but at 
> least more failure cases. Would likely require quite some changes to 
> fsm, but that's not exactly a bad thing (we've been slowly rewriting 
> the damn thing anyway
> :)
> Yes, exactly my though above :)
> Best Regards,
> Elena.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Improving-scriptlet-related-rpm-plugin-hooks.patch
Type: application/octet-stream
Size: 11068 bytes
Desc: not available
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20121128/bc7047a4/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 7220 bytes
Desc: not available
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20121128/bc7047a4/attachment-0001.p7s>

More information about the Rpm-maint mailing list