From 1fbf1905544142e460e5221d63361869a126ded7 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 25 Apr 2013 14:24:42 -0600 Subject: [PATCH] build: avoid unsafe functions in libgen.h POSIX says that both basename() and dirname() may return static storage (aka they need not be thread-safe); and that they may but not must modify their input argument. Furthermore, 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 thread-safe 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 --- 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 ebb6613d9f..f0f78f5f2d 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 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 ' \ + $(_sc_search_regexp) + # raw xmlGetProp requires some nasty casts sc_prohibit_xmlGetProp: @prohibit='\ #include +#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 c61e280225..7a9436f860 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 #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 cf2f790a42..271f3709dc 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 #include #include -#include #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 40da306612..1faf1aed36 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -26,6 +26,7 @@ #include #include +#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 5a2a75c9a3..4c836c7569 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 #include #include -#include #include #define __VIR_CGROUP_ALLOW_INCLUDE_PRIV_H__ diff --git a/src/util/virpci.c b/src/util/virpci.c index e58010b42e..5811f22c05 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -36,6 +36,7 @@ #include #include +#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 d7b4508467..ffe7a54039 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; -- GitLab