Hi, 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, Elena. -----Original Message----- From: Panu Matilainen [mailto:pmati...@laiskiainen.org] Sent: Tuesday, November 27, 2012 12:26 PM To: Reshetova, Elena Cc: rpm-maint@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, script_type); if (rc != RPMRC_FAIL) { if (script_type == SCRIPTLET_TYPE_INTERNAL) rc = runLuaScript(...); else 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]; RPMPLUGINS_SET_HOOK_FUNC(PLUGINHOOK_SCRIPTLET_PRE); 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 return? >> 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 acronym... > 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 all). > 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. >
0001-Improving-scriptlet-related-rpm-plugin-hooks.patch
Description: Binary data
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
