Found by clang. Clang complained that virStorageBackendProbeTarget could dereference NULL if backingStoreFormat was NULL, but since all callers passed a valid pointer, I added attributes instead of null checks. * src/storage/storage_backend.c (virStorageBackendQEMUImgBackingFormat): Kill dead store. * src/storage/storage_backend_fs.c (virStorageBackendProbeTarget): Likewise. Skip null checks, by adding attributes. --- Thankfully, the null dereference scenario noted by clang was never triggered in the code, which is good since it was introduced as part of fixing a CVE. src/storage/storage_backend.c | 3 +-- src/storage/storage_backend_fs.c | 34 +++++++++++++++------------------- 2 files changed, 16 insertions(+), 21 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index d989743..1fe7ba6 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -581,7 +581,6 @@ static int virStorageBackendQEMUImgBackingFormat(const char *qemuimg) int newstdout = -1; char *help = NULL; enum { MAX_HELP_OUTPUT_SIZE = 1024*8 }; - int len; char *start; char *end; char *tmp; @@ -591,7 +590,7 @@ static int virStorageBackendQEMUImgBackingFormat(const char *qemuimg) &child, -1, &newstdout, NULL, VIR_EXEC_CLEAR_CAPS) < 0) goto cleanup; - if ((len = virFileReadLimFD(newstdout, MAX_HELP_OUTPUT_SIZE, &help)) < 0) { + if (virFileReadLimFD(newstdout, MAX_HELP_OUTPUT_SIZE, &help) < 0) { virReportSystemError(errno, _("Unable to read '%s -h' output"), qemuimg); diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 0761bd8..b6b8fdd 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -48,7 +48,7 @@ #define VIR_FROM_THIS VIR_FROM_STORAGE -static int +static int ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) virStorageBackendProbeTarget(virStorageVolTargetPtr target, char **backingStore, int *backingStoreFormat, @@ -59,10 +59,8 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target, int fd, ret; virStorageFileMetadata meta; - if (backingStore) - *backingStore = NULL; - if (backingStoreFormat) - *backingStoreFormat = VIR_STORAGE_FILE_AUTO; + *backingStore = NULL; + *backingStoreFormat = VIR_STORAGE_FILE_AUTO; if (encryption) *encryption = NULL; @@ -75,7 +73,7 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target, allocation, capacity)) < 0) { close(fd); - return -1; + return ret; } memset(&meta, 0, sizeof(meta)); @@ -95,20 +93,19 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target, close(fd); if (meta.backingStore) { - if (backingStore) { - *backingStore = meta.backingStore; - meta.backingStore = NULL; - if (meta.backingStoreFormat == VIR_STORAGE_FILE_AUTO) { - if ((*backingStoreFormat = virStorageFileProbeFormat(*backingStore)) < 0) { - close(fd); - goto cleanup; - } - } else { - *backingStoreFormat = meta.backingStoreFormat; + *backingStore = meta.backingStore; + meta.backingStore = NULL; + if (meta.backingStoreFormat == VIR_STORAGE_FILE_AUTO) { + if ((*backingStoreFormat + = virStorageFileProbeFormat(*backingStore)) < 0) { + close(fd); + goto cleanup; } } else { - VIR_FREE(meta.backingStore); + *backingStoreFormat = meta.backingStoreFormat; } + } else { + VIR_FREE(meta.backingStore); } if (capacity && meta.capacity) @@ -139,8 +136,7 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target, return 0; cleanup: - if (backingStore) - VIR_FREE(*backingStore); + VIR_FREE(*backingStore); return -1; } -- 1.7.2