[libvirt] [PATCH] Fix qemuMigrationToFile nonull annotation
The qemuMigrationToFile method was accidentally annotated for the 'compressor' parameter to be non-null, instead of the 'path' parameter. Thus GCC with -O2, unhelpfully deleted the entire 'if (compressor == NULL)' block of code during optimization. Thus NULL was passed to virCommandNew() with predictably bad results. * src/qemu/qemu_migration.h: Fix non-null annotation to be against path instead of compressor --- src/qemu/qemu_migration.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) This shows that the 'ATTRIBUTE_NONNULL' annotation is actually really very dangerous to use. GCC is incapable of issuing any warnings about callers passing NULL, unless they pass a literal "NULL". If the caller does 'void *p = NULL; foo(p)' it will not warn. GCC is also not warning about the fact that there is a huge block of code for 'if (compressor == NULL)' that is "dead" code and being deleted. While it is perhaps nice to have ATTRIBUTE_NONNULL for static analysis tools like clang, IMHO, it is too dangerous for us to continue to have it enabled in builds. I think we should define it to a no-op macro, unless explicitly enabled with configure. diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index f4e86c8..c0f3aa2 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -64,7 +64,7 @@ int qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr vm, int fd, off_t offset, const char *path, const char *compressor, bool is_reg, bool bypassSecurityDriver) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(6) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK; #endif /* __QEMU_MIGRATION_H__ */ -- 1.7.4.4
On 05/05/2011 04:38 AM, Daniel P. Berrange wrote:
The qemuMigrationToFile method was accidentally annotated for the 'compressor' parameter to be non-null, instead of the 'path' parameter. Thus GCC with -O2, unhelpfully deleted the entire 'if (compressor == NULL)' block of code during optimization. Thus NULL was passed to virCommandNew() with predictably bad results.
Regression introduced in commit 7c31e1e, so fortunately 0.9.1 is immune.
This shows that the 'ATTRIBUTE_NONNULL' annotation is actually really very dangerous to use. GCC is incapable of issuing any warnings about callers passing NULL, unless they pass a literal "NULL". If the caller does 'void *p = NULL; foo(p)' it will not warn. GCC is also not warning about the fact that there is a huge block of code for 'if (compressor == NULL)' that is "dead" code and being deleted.
While it is perhaps nice to have ATTRIBUTE_NONNULL for static analysis tools like clang, IMHO, it is too dangerous for us to continue to have it enabled in builds. I think we should define it to a no-op macro, unless explicitly enabled with configure.
Hmm, maybe you're right, although it should still be defined for cases when compiling with a static analyzer (such as how sa_assert is a no-op for gcc but does something important for clang). Maybe we should also file an enhancement request against gcc, to alter the syntax for attribute-nonnull. That is, void f(void *a, void *b) ATTRIBUTE_NONNULL(2,b) would be nicer, where gcc complains if parameter 2 is not named b, so that the mere deletion of parameter a catches this sort of mistake faster.
+++ b/src/qemu/qemu_migration.h @@ -64,7 +64,7 @@ int qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr vm, int fd, off_t offset, const char *path, const char *compressor, bool is_reg, bool bypassSecurityDriver) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(6) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK;
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel P. Berrange -
Eric Blake