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@xxxxxxxxxx +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list