Commit Graph

15 Commits

Author SHA1 Message Date
Eric Blake
cb9bd7963b virt-login-shell: silence coverity warning
Coverity spotted that 'nfdlist' (ssize_t) could be -1, but that we
were using 'i' (size_t) to iterate over the list at cleanup, with
crashing results because it promotes to a really big unsigned number.

* tools/virt-login-shell.c (main): Avoid treating -1 as unsigned.

Signed-off-by: Eric Blake <eblake@redhat.com>
2014-03-05 11:59:06 -07:00
Eric Blake
730fc9622b virt-login-shell: saner exit value
virt-login-shell was exiting with status 0, regardless of what the
wrapped shell returned.  This is unkind to users; we should behave
more like env(1), nice(1), su(1), and other wrapper programs, by
preserving the invoked application's status (which includes the
distinction between death due to signal vs. normal death).

* tools/virt-login-shell.c (main): Pass through child exit status.
* tools/virt-login-shell.pod: Document exit status.

Signed-off-by: Eric Blake <eblake@redhat.com>
2014-03-03 12:40:32 -07:00
Eric Blake
4594a33b4b virt-login-shell: use single instead of double fork
Note that 'virsh lxc-enter-namespace' must double-fork, for two
reasons: some namespaces can only be done from a single thread,
while virsh is multithreaded; and because virsh can be run in
batch mode where we must not corrupt the namespace of that
execution upon return from the subsidiary command.

When virt-login-shell was first written, it blindly copied from
'virsh lxc-enter-namespace', including the double-fork.  But
neither of the reasons for double forking apply to
virt-login-shell (we are single-threaded, and we have nothing to
do after the child completes that would require us to preserve a
namespace), so we can simplify life by using a single fork.
In turn, this will make it easier for a future patch to pass the
child's exit status on to the invoking shell.

In flattening to a single fork, note that closing the fds must
be done after fork, because the parent process still needs to
use fds to control the virConnectPtr; meanwhile, chdir can be
done prior to forking (in fact, it's easier to report errors
on anything attempted before forking).

* tools/virt-login-shell.c (main): Single rather than double fork.
(virLoginShellFini): Delete, by inlining actions instead.

Signed-off-by: Eric Blake <eblake@redhat.com>
2014-03-03 12:40:32 -07:00
Eric Blake
25f87817ab virFork: simplify semantics
The old semantics of virFork() violates the priciple of good
usability: it requires the caller to check the pid argument
after use, *even when virFork returned -1*, in order to properly
abort a child process that failed setup done immediately after
fork() - that is, the caller must call _exit() in the child.
While uses in virfile.c did this correctly, uses in 'virsh
lxc-enter-namespace' and 'virt-login-shell' would happily return
from the calling function in both the child and the parent,
leading to very confusing results. [Thankfully, I found the
problem by inspection, and can't actually trigger the double
return on error without an LD_PRELOAD library.]

It is much better if the semantics of virFork are impossible
to abuse.  Looking at virFork(), the parent could only ever
return -1 with a non-negative pid if it misused pthread_sigmask,
but this never happens.  Up until this patch series, the child
could return -1 with non-negative pid if it fails to set up
signals correctly, but we recently fixed that to make the child
call _exit() at that point instead of forcing the caller to do
it.  Thus, the return value and contents of the pid argument are
now redundant (a -1 return now happens only for failure to fork,
a child 0 return only happens for a successful 0 pid, and a
parent 0 return only happens for a successful non-zero pid),
so we might as well return the pid directly rather than an
integer of whether it succeeded or failed; this is also good
from the interface design perspective as users are already
familiar with fork() semantics.

One last change in this patch: before returning the pid directly,
I found cases where using virProcessWait unconditionally on a
cleanup path of a virFork's -1 pid return would be nicer if there
were a way to avoid it overwriting an earlier message.  While
such paths are a bit harder to come by with my change to a direct
pid return, I decided to keep the virProcessWait change in this
patch.

* src/util/vircommand.h (virFork): Change signature.
* src/util/vircommand.c (virFork): Guarantee that child will only
return on success, to simplify callers.  Return pid rather than
status, now that the situations are always the same.
(virExec): Adjust caller, also avoid open-coding process death.
* src/util/virprocess.c (virProcessWait): Tweak semantics when pid
is -1.
(virProcessRunInMountNamespace): Adjust caller.
* src/util/virfile.c (virFileAccessibleAs, virFileOpenForked)
(virDirCreate): Likewise.
* tools/virt-login-shell.c (main): Likewise.
* tools/virsh-domain.c (cmdLxcEnterNamespace): Likewise.
* tests/commandtest.c (test23): Likewise.

Signed-off-by: Eric Blake <eblake@redhat.com>
2014-03-03 12:40:32 -07:00
Eric Blake
c72e76c3d9 util: make it easier to grab only regular process exit
Right now, a caller waiting for a child process either requires
the child to have status 0, or must use WIFEXITED() and friends
itself.  But in many cases, we want the middle ground of treating
fatal signals as an error, and directly accessing the normal exit
value without having to use WEXITSTATUS(), in order to easily
detect an expected non-zero exit status.  This adds the middle
ground to the low-level virProcessWait; the next patch will add
it to virCommand.

* src/util/virprocess.h (virProcessWait): Alter signature.
* src/util/virprocess.c (virProcessWait): Add parameter.
(virProcessRunInMountNamespace): Adjust caller.
* src/util/vircommand.c (virCommandWait): Likewise.
* src/util/virfile.c (virFileAccessibleAs): Likewise.
* src/lxc/lxc_container.c (lxcContainerHasReboot)
(lxcContainerAvailable): Likewise.
* daemon/libvirtd.c (daemonForkIntoBackground): Likewise.
* tools/virt-login-shell.c (main): Likewise.
* tools/virsh-domain.c (cmdLxcEnterNamespace): Likewise.
* tests/testutils.c (virtTestCaptureProgramOutput): Likewise.
* tests/commandtest.c (test23): Likewise.

Signed-off-by: Eric Blake <eblake@redhat.com>
2014-03-03 12:40:31 -07:00
Thorsten Behrens
721949059b maint: align whitespaces with project conventions. 2014-01-20 14:35:08 +01:00
Eric Blake
3d007cb5f8 virt-login-shell: fix regressions in behavior
Our fixes for CVE-2013-4400 were so effective at "fixing" bugs
in virt-login-shell that we ended up fixing it into a useless
do-nothing program.

Commit 3e2f27e1 picked the name LIBVIRT_SETUID_RPC_CLIENT for
the witness macro when we are doing secure compilation.  But
commit 9cd6a57d checked whether the name IN_VIRT_LOGIN_SHELL,
from an earlier version of the patch series, was defined; with
the net result that virt-login-shell invariably detected that
it was setuid and failed virInitialize.

Commit b7fcc799 closed all fds larger than stderr, but in the
wrong place.  Looking at the larger context, we mistakenly did
the close in between obtaining the set of namespace fds, then
actually using those fds to switch namespace, which means that
virt-login-shell will ALWAYS fail.

This is the minimal patch to fix the regressions, although
further patches are also worth having to clean up poor
semantics of the resulting program (for example, it is rude to
not pass on the exit status of the wrapped program back to the
invoking shell).

* tools/virt-login-shell.c (main): Don't close fds until after
namespace swap.
* src/libvirt.c (virGlobalInit): Use correct macro.

Signed-off-by: Eric Blake <eblake@redhat.com>
2014-01-09 15:05:04 -07:00
Eric Blake
7cc3a7189c virt-login-shell: clean up usage
I noticed a few odd things in 'virt-login-shell --help' output.

* tools/virt-login-shell.c (usage): At most one option accepted,
drop trailing colon.

Signed-off-by: Eric Blake <eblake@redhat.com>
2013-12-23 21:53:48 -07:00
Daniel P. Berrange
d665003da1 Set a sane $PATH for virt-login-shell
The virt-login-shell binary shouldn't need to execute programs
relying on $PATH, but just in case set a fixed $PATH value
of /bin:/usr/bin

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
2013-10-21 14:03:52 +01:00
Daniel P. Berrange
b7fcc799ad Close all non-stdio FDs in virt-login-shell (CVE-2013-4400)
We don't want to inherit any FDs in the new namespace
except for the stdio FDs. Explicitly close them all,
just in case some do not have the close-on-exec flag
set.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
2013-10-21 14:03:52 +01:00
Ruben Kerkhof
11cdc424d3 virt-login-shell: improve error message grammar
and wrap some long lines

Signed-off-by: Eric Blake <eblake@redhat.com>
2013-08-13 17:28:06 -06:00
Daniel P. Berrange
a396473494 Address missed feedback from review of virt-login-shell
Address a number of code, style and docs issues identified
in review of virt-login-shell after it was merged.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
2013-08-13 14:06:01 +01:00
Daniel P. Berrange
ac692e3af2 Fix double-free and broken logic in virt-login-shell
The virLoginShellAllowedUser method must not free the 'groups'
parameter it is given, as that is owned by the caller.

The virLoginShellAllowedUser method should be checking
'!*ptr' (ie empty string) rather than '!ptr' (NULL string)
since the latter cannot be true.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
2013-08-09 12:55:59 +01:00
Jim Fehlig
26b8a4dd23 build: fix compilation of virt-login-shell.c
virt-login-shell.c was failing to compile with

CC       virt_login_shell-virt-login-shell.o
virt-login-shell.c: In function 'main':
virt-login-shell.c:205:5: error: implicit declaration of function 'setlocale' [-Werror=implicit-function-declaration]
virt-login-shell.c:205:5: error: nested extern declaration of 'setlocale' [-Werror=nested-externs]
virt-login-shell.c:205:20: error: 'LC_ALL' undeclared (first use in this function)
2013-08-08 13:53:25 -06:00
Dan Walsh
54d69f540c Introduce a virt-login-shell binary
Add a virt-login-shell binary that can be set as a user's
shell, such that when they login, it causes them to enter
the LXC container with a name matching their user name.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
2013-08-08 16:36:31 +01:00