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

Re: [libvirt] [PATCH] build: avoid unsafe functions in libgen.h



On 04/25/2013 04:30 PM, Eric Blake wrote:
> POSIX says that both basename() and dirname() may return static
> storage (aka they are not thread-safe); and that they may but
> not must modify their input argument.  Furthermore, <libgen.h>
> is not available on all platforms.  For these reasons, you should
> never use these functions in a multi-threaded library.
>
> Gnulib instead recommends a way to avoid the portability nightmare:
> gnulib's "dirname.h" provides useful counterparts.  The obvious
> dir_name() and base_name() are GPL (because they malloc(), but call
> exit() on failure) so we can't use them; but the LGPL variants
> mdir_name() (malloc's or returns NULL) and last_component (always
> points into the incoming string without modifying it, differing
> from basename semantics only on corner cases like the empty string
> that we shouldn't be hitting in the first place) are already in use
> in libvirt.  This finishes the swap over to the safe functions.
>
> * cfg.mk (sc_prohibit_libgen): New rule.
> * src/util/vircgroup.c: Fix offenders.
> * src/parallels/parallels_storage.c (parallelsPoolAddByDomain):
> Likewise.
> * src/parallels/parallels_network.c (parallelsGetBridgedNetInfo):
> Likewise.
> * src/node_device/node_device_udev.c (udevProcessSCSIHost)
> (udevProcessSCSIDevice): Likewise.
> * src/storage/storage_backend_disk.c
> (virStorageBackendDiskDeleteVol): Likewise.
> * src/util/virpci.c (virPCIGetDeviceAddressFromSysfsLink):
> Likewise.
> * src/util/virstoragefile.h (_virStorageFileMetadata): Avoid false
> positive.
>
> Signed-off-by: Eric Blake <eblake redhat com>
> ---
>
> Spotted during a review of Laine's pending work.
>
>  cfg.mk                             | 6 ++++++
>  src/node_device/node_device_udev.c | 7 ++++---
>  src/parallels/parallels_network.c  | 4 +++-
>  src/parallels/parallels_storage.c  | 8 ++++----
>  src/storage/storage_backend_disk.c | 5 +++--
>  src/util/vircgroup.c               | 3 +--
>  src/util/virpci.c                  | 3 ++-
>  src/util/virstoragefile.h          | 2 +-
>  8 files changed, 24 insertions(+), 14 deletions(-)
>
> diff --git a/cfg.mk b/cfg.mk
> index ebb6613..f0f78f5 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -493,6 +493,12 @@ sc_prohibit_gethostby:
>  	halt='use getaddrinfo, not gethostby*'				\
>  	  $(_sc_search_regexp)
>
> +# dirname and basename from <libgen.h> are not required to be thread-safe
> +sc_prohibit_libgen:
> +	@prohibit='( (base|dir)name *\(|include .libgen\.h)'		\
> +	halt='use functions from gnulib "dirname.h", not <libgen.h>'	\
> +	  $(_sc_search_regexp)
> +
>  # raw xmlGetProp requires some nasty casts
>  sc_prohibit_xmlGetProp:
>  	@prohibit='\<xmlGetProp *\('					\
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index 92292be..f98841e 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1,7 +1,7 @@
>  /*
>   * node_device_udev.c: node device enumeration - libudev implementation
>   *
> - * Copyright (C) 2009-2012 Red Hat, Inc.
> + * Copyright (C) 2009-2013 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
> @@ -26,6 +26,7 @@
>  #include <scsi/scsi.h>
>  #include <c-ctype.h>
>
> +#include "dirname.h"
>  #include "node_device_udev.h"
>  #include "virerror.h"
>  #include "node_device_conf.h"
> @@ -653,7 +654,7 @@ static int udevProcessSCSIHost(struct udev_device *device ATTRIBUTE_UNUSED,
>      union _virNodeDevCapData *data = &def->caps->data;
>      char *filename = NULL;
>
> -    filename = basename(def->sysfs_path);
> +    filename = last_component(def->sysfs_path);
>
>      if (!STRPREFIX(filename, "host")) {
>          VIR_ERROR(_("SCSI host found, but its udev name '%s' does "
> @@ -774,7 +775,7 @@ static int udevProcessSCSIDevice(struct udev_device *device ATTRIBUTE_UNUSED,
>      union _virNodeDevCapData *data = &def->caps->data;
>      char *filename = NULL, *p = NULL;
>
> -    filename = basename(def->sysfs_path);
> +    filename = last_component(def->sysfs_path);
>
>      if (udevStrToLong_ui(filename, &p, 10, &data->scsi.host) == -1) {
>          goto out;
> diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c
> index c61e280..7a9436f 100644
> --- a/src/parallels/parallels_network.c
> +++ b/src/parallels/parallels_network.c
> @@ -2,6 +2,7 @@
>   * parallels_storage.c: core privconn functions for managing
>   * Parallels Cloud Server hosts
>   *
> + * Copyright (C) 2013 Red Hat, Inc.
>   * Copyright (C) 2012 Parallels, Inc.
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -23,6 +24,7 @@
>  #include <config.h>
>
>  #include "datatypes.h"
> +#include "dirname.h"
>  #include "viralloc.h"
>  #include "virerror.h"
>  #include "md5.h"
> @@ -64,7 +66,7 @@ static int parallelsGetBridgedNetInfo(virNetworkDefPtr def, virJSONValuePtr jobj
>          goto cleanup;
>      }
>
> -    if (!(def->bridge = strdup(basename(bridgePath)))) {
> +    if (!(def->bridge = strdup(last_component(bridgePath)))) {
>          virReportOOMError();
>          goto cleanup;
>      }
> diff --git a/src/parallels/parallels_storage.c b/src/parallels/parallels_storage.c
> index cf2f790..271f370 100644
> --- a/src/parallels/parallels_storage.c
> +++ b/src/parallels/parallels_storage.c
> @@ -2,6 +2,7 @@
>   * parallels_storage.c: core driver functions for managing
>   * Parallels Cloud Server hosts
>   *
> + * Copyright (C) 2013 Red Hat, Inc.
>   * Copyright (C) 2012 Parallels, Inc.
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -28,9 +29,9 @@
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <unistd.h>
> -#include <libgen.h>
>
>  #include "datatypes.h"
> +#include "dirname.h"
>  #include "viralloc.h"
>  #include "configmake.h"
>  #include "virstoragefile.h"
> @@ -230,13 +231,12 @@ parallelsPoolAddByDomain(virConnectPtr conn, virDomainObjPtr dom)
>      virStoragePoolObjPtr pool = NULL;
>      int j;
>
> -    if (!(poolPath = strdup(pdom->home))) {
> +    poolPath = mdir_name(pdom->home);
> +    if (!poolPath) {
>          virReportOOMError();
>          return NULL;
>      }
>
> -    poolPath = dirname(poolPath);
> -
>      for (j = 0; j < pools->count; j++) {
>          if (STREQ(poolPath, pools->objs[j]->def->target.path)) {
>              pool = pools->objs[j];
> diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c
> index 40da306..1faf1ae 100644
> --- a/src/storage/storage_backend_disk.c
> +++ b/src/storage/storage_backend_disk.c
> @@ -26,6 +26,7 @@
>  #include <unistd.h>
>  #include <stdio.h>
>
> +#include "dirname.h"
>  #include "virerror.h"
>  #include "virlog.h"
>  #include "storage_backend_disk.h"
> @@ -728,8 +729,8 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED,
>          goto cleanup;
>      }
>
> -    dev_name = basename(devpath);
> -    srcname = basename(pool->def->source.devices[0].path);
> +    dev_name = last_component(devpath);
> +    srcname = last_component(pool->def->source.devices[0].path);
>      VIR_DEBUG("dev_name=%s, srcname=%s", dev_name, srcname);
>
>      isDevMapperDevice = virIsDevMapperDevice(devpath);
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index 5a2a75c..4c836c7 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -1,7 +1,7 @@
>  /*
>   * vircgroup.c: methods for managing control cgroups
>   *
> - * Copyright (C) 2010-2012 Red Hat, Inc.
> + * Copyright (C) 2010-2013 Red Hat, Inc.
>   * Copyright IBM Corp. 2008
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -37,7 +37,6 @@
>  #include <sys/stat.h>
>  #include <sys/types.h>
>  #include <signal.h>
> -#include <libgen.h>
>  #include <dirent.h>
>
>  #define __VIR_CGROUP_ALLOW_INCLUDE_PRIV_H__
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index e58010b..5811f22 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -36,6 +36,7 @@
>  #include <unistd.h>
>  #include <stdlib.h>
>
> +#include "dirname.h"
>  #include "virlog.h"
>  #include "viralloc.h"
>  #include "vircommand.h"
> @@ -1925,7 +1926,7 @@ virPCIGetDeviceAddressFromSysfsLink(const char *device_link,
>          return ret;
>      }
>
> -    config_address = basename(device_path);
> +    config_address = last_component(device_path);
>      if (VIR_ALLOC(*bdf) != 0) {
>          virReportOOMError();
>          goto out;
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index d7b4508..ffe7a54 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -56,7 +56,7 @@ typedef virStorageFileMetadata *virStorageFileMetadataPtr;
>  struct _virStorageFileMetadata {
>      char *backingStore; /* Canonical name (absolute file, or protocol) */
>      char *backingStoreRaw; /* If file, original name, possibly relative */
> -    char *directory; /* The directory containing basename(backingStoreRaw) */
> +    char *directory; /* The directory containing basename of backingStoreRaw */
>      int backingStoreFormat; /* enum virStorageFileFormat */
>      bool backingStoreIsFile;
>      virStorageFileMetadataPtr backingMeta;
>

ACK. Thanks!


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