[libvirt] [PATCH 1/2] Implementation deficiency in virInitctlSetRunLevel v4

[Eric Blake]

On 12/20/2013 09:34 AM, Daniel P. Berrange wrote:
> On Fri, Dec 20, 2013 at 08:24:41PM +0400, Reco wrote:
>> Implement virProcessRunInMountNamespace, which runs callback of type
>> virProcessNamespaceCallback in a container namespace.
>>
>> Hope it'll nail it this time.

Fails 'make syntax-check':

prohibit_fork_wrappers
src/util/virprocess.c:874:    switch (cpid = fork()) {
maint.mk: use virCommand for child processes
make: *** [sc_prohibit_fork_wrappers] Error 1

>> +
>> +    switch (cpid = fork()) {
>> +    case 0:
>> +        if (setns(fd, 0) == -1) {
>> +            _exit(-1);

exit(-1) is wrong; the user will see $? of 255 (not -1), and it is not
our typical exit status for failure (where we normally want to be < 128
so that it is not confused with exit due to signal).  Besides, I hate
magic numbers; better would be _exit(EXIT_FAILURE).

>> +    default:
>> +        if (virProcessWait(cpid, &status) < 0 || status < 0) {

No need to pass &status to virProcessWait() if you are going to insist
on 0 status anyways; better to pass NULL and let virProcessWait do the
validation on your behalf.

>> +            virReportSystemError(errno,
>> +                                 _("Callback failed with status %i"),
>> +                                 status);

This overwrites the (better) error message that virProcessWait() already
generated.

>> +            ret = 1;

This returns a number > 0 on failure, even though our normal conventions
is to return a negative number on failure.

>> +++ b/src/util/virprocess.h
>> @@ -60,4 +60,10 @@ int virProcessSetNamespaces(size_t nfdlist,
>>  int virProcessSetMaxMemLock(pid_t pid, unsigned long long bytes);
>>  int virProcessSetMaxProcesses(pid_t pid, unsigned int procs);
>>  int virProcessSetMaxFiles(pid_t pid, unsigned int files);
>> +
>> +typedef int (*virProcessNamespaceCallback)(pid_t pid, void *opaque);

Worth documenting that the return value of this callback is passed on to
_exit() (and thus that errors should be EXIT_FAILURE rather than our
typical -1 on failure).

I'll repost a full v5 series with my proposed fixes to these issues in a
separate thread, and wait for a review (since this fixes a CVE, it needs
a second set of eyes).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20131220/ce04493a/attachment-0001.sig>