[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v4 6/7] util: Add helpers for safe domain console operations



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 redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]