[libvirt] [PATCH v4 6/7] util: Add helpers for safe domain console operations
Eric Blake
eblake at redhat.com
Wed Feb 22 22:19:31 UTC 2012
On 02/06/2012 06:50 AM, Peter Krempa wrote:
> This patch adds a set of functions used in creating console streams for
> domains using PTYs and ensures mutually exclusive access to the PTYs.
>
> If mutualy exclusive access is not used, two clients may open the same
s/mutualy/mutually/
> console, which results in corruption on both clients as both of them
> race to read data from the PTY.
>
> Two approaches are used to ensure this:
> 1) Internal data structure holding open PTYs.
> This is used internally and enables the user to forcibly
> terminate another console connection eg. when somebody leaves
> the console open on another host.
>
> 2) UUCP style lock files:
> This uses UUCP lock files according to the FHS
> ( http://www.pathname.com/fhs/pub/fhs-2.3.html#VARLOCKLOCKFILES )
> to check if other programs (like minicom) are not using the pty
> device of the console.
>
> This feature is disabled by default and may be enabled using
> configure parameter
> --with-console-lock-files=/path/to/lock/file/directory
> or --with-console-lock-files=auto (which tries to infer the
> location from OS used (currently only linux).
Should we also be modifying libvirt.spec.in to enable console lock files
when building the RPM for Fedora?
>
> On usual linux systems, normal users may not write to the
> /var/lock directory containing the locks. This poses problems
> while in session mode. If the current user has no access to the
> lockfile directory, check for presence of the file is still
> done, but no lock file is created. This does NOT result in an
> error.
> ---
> configure.ac | 39 ++++-
> po/POTFILES.in | 1 +
> src/Makefile.am | 6 +-
> src/conf/virconsole.c | 396 ++++++++++++++++++++++++++++++++++++++++++++++
> src/conf/virconsole.h | 36 ++++
> src/libvirt_private.syms | 6 +
> 6 files changed, 475 insertions(+), 9 deletions(-)
> create mode 100644 src/conf/virconsole.c
> create mode 100644 src/conf/virconsole.h
>
> diff --git a/configure.ac b/configure.ac
> index 9fb7bfc..3254105 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -327,6 +327,12 @@ AC_ARG_WITH([remote],
> AC_HELP_STRING([--with-remote], [add remote driver support @<:@default=yes@:>@]),[],[with_remote=yes])
> AC_ARG_WITH([libvirtd],
> AC_HELP_STRING([--with-libvirtd], [add libvirtd support @<:@default=yes@:>@]),[],[with_libvirtd=yes])
> +AC_ARG_WITH([console-lock-files],
> + AC_HELP_STRING([--with-console-lock-files],
> + [location for UUCP style lock files for console PTYs
> + (use auto for default paths on some platforms)
> + @<:@default=disabled@:>@]),
> + [],[with_console_lock_files=disabled])
If I say ./configure --without-console-lock-files, then that defaults
$with_console_lock_files=no.
For consistency, I'd rather see the default be 'no', not 'disabled';
that way, you take advantage of autoconf's automatic --without-* support.
Conversely, if I say ./configure --with-console-lock-files without an
argument, autoconf defaults that to yes. I think you have to handle
'auto' and 'yes' in a similar manner, except the difference is that auto
can gracefully degrade to 'no', while 'yes' errors out if we can't find
a default location.
>
> dnl
> dnl in case someone want to build static binaries
> @@ -1197,6 +1203,22 @@ AM_CONDITIONAL([HAVE_AUDIT], [test "$with_audit" = "yes"])
> AC_SUBST([AUDIT_CFLAGS])
> AC_SUBST([AUDIT_LIBS])
>
> +dnl UUCP style file locks for PTY consoles
> +if test "$with_console_lock_files" != "disabled"; then
> + if test "$with_console_lock_files" = "auto"; then
> + dnl Default locations for platforms
> + if test "$with_linux" = "yes"; then
> + with_console_lock_files=/var/lock
> + fi
> + fi
> + if test "$with_console_lock_files" = "auto"; then
> + AC_MSG_ERROR([You must specify path for the lock files on this platform])
> + fi
> + AC_DEFINE_UNQUOTED([VIR_PTY_LOCK_FILE_PATH], "$with_console_lock_files",
> + [path to directory containing UUCP pty lock files])
> +fi
So here, I would use:
if test "$with_console_lock_files" != no; then
if test "$with_linux" = yes; then
with_console_lock_files=/var/lock
elif test "$with_console_lock_files = yes; then
AC_MSG_ERROR([console lock files requested, but no path given])
elif test "$with_console_lock_files" = auto; then
with_console_lock_files=no
fi
fi
if test "$with_console_lock_files" != no; then
AC_DEFINE_UNQUOTED([VIR_PTY_LOCK_FILE_PATH], ...)
fi
> +AM_CONDITIONAL([VIR_PTY_LOCK_FILE_PATH], [test "$with_console_lock_files" != "disabled"])
and again, I'd compare to 'no', not 'disabled'
> +
>
> dnl SELinux
> AC_ARG_WITH([selinux],
> @@ -2751,14 +2773,15 @@ AC_MSG_NOTICE([ Alloc OOM: $enable_oom])
> AC_MSG_NOTICE([])
> AC_MSG_NOTICE([Miscellaneous])
> AC_MSG_NOTICE([])
> -AC_MSG_NOTICE([ Debug: $enable_debug])
> -AC_MSG_NOTICE([ Warnings: $enable_compile_warnings])
> -AC_MSG_NOTICE([Warning Flags: $WARN_CFLAGS])
> -AC_MSG_NOTICE([ Readline: $lv_use_readline])
> -AC_MSG_NOTICE([ Python: $with_python])
> -AC_MSG_NOTICE([ DTrace: $with_dtrace])
> -AC_MSG_NOTICE([ XML Catalog: $XML_CATALOG_FILE])
> -AC_MSG_NOTICE([ Init script: $with_init_script])
> +AC_MSG_NOTICE([ Debug: $enable_debug])
> +AC_MSG_NOTICE([ Warnings: $enable_compile_warnings])
> +AC_MSG_NOTICE([ Warning Flags: $WARN_CFLAGS])
> +AC_MSG_NOTICE([ Readline: $lv_use_readline])
> +AC_MSG_NOTICE([ Python: $with_python])
> +AC_MSG_NOTICE([ DTrace: $with_dtrace])
> +AC_MSG_NOTICE([ XML Catalog: $XML_CATALOG_FILE])
> +AC_MSG_NOTICE([ Init script: $with_init_script])
> +AC_MSG_NOTICE([Console PTY locks: $with_console_lock_files])
Rather than reformat everything, why not just call the new entry
AC_MSG_NOTICE([Console locks: $with_console_lock_files])
so that you line up with te remaining lines and match the configure
option name.
> +++ b/src/conf/virconsole.c
> @@ -0,0 +1,396 @@
> +/**
> + * virconsole.c: api to guarantee mutualy exclusive
s/mutualy/mutually/
> + * access to domain's consoles
> + *
> + * Copyright (C) 2011-2012 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
We really ought to scrub our source tree (as an independent patch) to
use the FSF's preferred LGPL header (they now list a web URL instead of
a street address).
> +static char *virConsoleLockFilePath(const char *pty)
> +{
> + char *path = NULL;
> + char *sanitizedPath = NULL;
> + char *ptyCopy;
> + char *filename;
> + char *p;
> +
> + if (!(ptyCopy = strdup(pty))) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + if ((filename = strstr(ptyCopy, "/dev/")))
> + filename += 5; /* skip /dev/ anywhere in the path*/
That's dangerous - you are skipping the first 5 bytes, even if /dev/
occurred somewhere other than the first 5 bytes. I think you really
only want to skip things if ptyCopy starts with, rather than contains, /dev.
> +
> + /* substitute path forward slashes for underscores */
> + p = filename;
I would consider using:
p = STRSKIP(ptyCopy, "/dev/");
if (!p)
p = ptyCopy;
filename = p;
> +static int virConsoleLockFileCreate(const char *pty)
> +{
> + char *path = NULL;
> + int ret = -1;
> + int lockfd = -1;
> + char *pidStr = NULL;
> + int pid;
Shouldn't this be pid_t?
> + /* build lock file path */
> + if (!(path = virConsoleLockFilePath(pty)))
> + goto cleanup;
> +
> +
> +
Why so many blank lines?
> + /* check if a log file and process holding the lock still exists */
double space.
> + if (virPidFileReadPathIfAlive(path, &pid, NULL) == 0 && pid >= 0) {
> + /* the process exists, the lockfile is valid */
> + virConsoleError(VIR_ERR_OPERATION_FAILED,
> + _("Requested console pty '%s' is locked by "
> + "lock file '%s' held by process %d"),
> + pty, path, pid);
You must use %lld and (long long) pid when printing pid_t values, for
the sake of mingw64 (hmm, that reminds me - I have pending patches that
need a review:
https://www.redhat.com/archives/libvir-list/2012-February/msg00559.html)
> + goto cleanup;
> + } else {
> + /* clean up the stale/corrupted/nonexistent lockfile */
> + unlink(path);
> + }
> + /* lockfile doesn't (shouldn't) exist */
> +
> + /* ensure correct format according to filesystem hierarchy standard */
> + /* http://www.pathname.com/fhs/pub/fhs-2.3.html#VARLOCKLOCKFILES */
> + if (virAsprintf(&pidStr, "%10d\n", (int) getpid()) < 0)
Again, %10lld, and (long long) getpid(), for mingw64 (yes, a 64-bit pid
will overflow 10 bytes, but that's only on systems that aren't compliant
to the FHS in the first place).
> +/**
> + * Allocate structures for storing information about active console streams
> + * in domain's private data section.
> + *
> + * Returns pointer to the allocated structure or NULL on error
> + */
> +virConsolesPtr virConsoleAlloc(void)
> +{
> + virConsolesPtr cons;
> + if (VIR_ALLOC(cons) < 0)
> + return NULL;
> +
> + /* there will hardly be any consoles most of the time, the hash
> + * does not have to be huge */
> + if (!(cons->hash = virHashCreate(3, virConsoleHashEntryFree)))
> + goto error;
> +
> + if (virMutexInit(&cons->lock) < 0)
> + goto error;
Bug - if you fail to init a mutex, then error will call virConsoleFree,
which will then attempt to lock your uninit mutex. It is only safe to
call virConsoleFree() on cleanup if the mutex was created.
> +
> + return cons;
> +error:
> + virConsoleFree(cons);
> + return NULL;
> +}
> +
> +/**
> + * Free structures for handling open console streams.
> + *
> + * @cons Pointer to the private structure.
> + */
> +void virConsoleFree(virConsolesPtr cons)
> +{
> + if (!cons || !cons->hash)
> + return;
> +
> + virMutexLock(&cons->lock);
> + virHashFree(cons->hash);
> + cons->hash = NULL;
Dead assignment, since you are freeing cons a few lines later.
> +int virConsoleOpen(virConsolesPtr cons,
> + const char *pty,
> + virStreamPtr st,
> + bool force)
> +{
> + virConsoleStreamInfoPtr cbdata = NULL;
> + virStreamPtr savedStream;
> + int ret;
> +
> + virMutexLock(&cons->lock);
> +
> + if ((savedStream = virHashLookup(cons->hash, pty))) {
> + if (!force) {
> + /* entry found, console is busy */
> + virMutexUnlock(&cons->lock);
> + return 1;
> + } else {
> + /* terminate existing connection */
> + virFDStreamSetInternalCloseCb(savedStream, NULL, NULL, NULL);
> + virStreamAbort(savedStream);
I'm not quite sure why you want to unregister any internal callback
prior to aborting the stream. I"m not saying it's wrong, just that I
didn't follow it.
> + virHashRemoveEntry(cons->hash, pty);
> + /* continue adding a new stream connection */
> + }
> + }
> +
> + /* create the lock file */
> + if ((ret = virConsoleLockFileCreate(pty)) < 0) {
> + virMutexUnlock(&cons->lock);
> + return ret;
> + }
> +
> + /* obtain a reference to the stream */
> + if (virStreamRef(st) < 0) {
> + virMutexUnlock(&cons->lock);
> + return -1;
> + }
> +
> + if (VIR_ALLOC(cbdata) < 0)
> + goto error;
It looks like this function is responsible for raising an error message
on all failure paths, which means you are missing virReportOOMError() here.
> +
> + if (virHashAddEntry(cons->hash, pty, st) < 0)
> + goto error;
> +
> + cbdata->cons = cons;
> + if (!(cbdata->pty = strdup(pty)))
> + goto error;
and another virReportOOMError().
> +++ b/src/conf/virconsole.h
> @@ -0,0 +1,36 @@
> +/**
> + * virconsole.h: api to guarantee mutualy exclusive
s/mutualy/mutually/
Probably worth a v5.
--
Eric Blake eblake at 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: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120222/b00e7972/attachment-0001.sig>
More information about the libvir-list
mailing list