[libvirt] [PATCH] build: avoid unsafe functions in libgen.h
Laine Stump
laine at laine.org
Thu Apr 25 20:41:38 UTC 2013
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 at 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!
More information about the libvir-list
mailing list