[PULL 4/4] qapi/error: Check format string argument in error_*prepend()

Markus Armbruster posted 4 patches 5 years, 3 months ago
Maintainers: Markus Armbruster <armbru@redhat.com>
[PULL 4/4] qapi/error: Check format string argument in error_*prepend()
Posted by Markus Armbruster 5 years, 3 months ago
From: Philippe Mathieu-Daudé <philmd@redhat.com>

error_propagate_prepend() "behaves like error_prepend()", and
error_prepend() uses "formatting @fmt, ... like printf()".
error_prepend() checks its format string argument, but
error_propagate_prepend() does not. Fix by addint the format
attribute to error_propagate_prepend() and error_vprepend().

This would have caught the bug fixed in the previous commit.

Missed in commit 4b5766488f "error: Fix use of error_prepend() with
&error_fatal, &error_abort".

Inspired-by: Stefan Weil <sw@weilnetz.de>
Suggested-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefan Weil <sw@weilnetz.de>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20200723171205.14949-1-philmd@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
[Commit message tweaked]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/error.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 7932594dce..eaa05c4837 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -382,13 +382,15 @@ void error_propagate(Error **dst_errp, Error *local_err);
  * Please use ERRP_GUARD() and error_prepend() instead when possible.
  */
 void error_propagate_prepend(Error **dst_errp, Error *local_err,
-                             const char *fmt, ...);
+                             const char *fmt, ...)
+    GCC_FMT_ATTR(3, 4);
 
 /*
  * Prepend some text to @errp's human-readable error message.
  * The text is made by formatting @fmt, @ap like vprintf().
  */
-void error_vprepend(Error *const *errp, const char *fmt, va_list ap);
+void error_vprepend(Error *const *errp, const char *fmt, va_list ap)
+    GCC_FMT_ATTR(2, 0);
 
 /*
  * Prepend some text to @errp's human-readable error message.
-- 
2.26.2


Re: [PULL 4/4] qapi/error: Check format string argument in error_*prepend()
Posted by Daniel P. Berrangé 5 years, 3 months ago
On Fri, Jul 24, 2020 at 03:47:04PM +0200, Markus Armbruster wrote:
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> error_propagate_prepend() "behaves like error_prepend()", and
> error_prepend() uses "formatting @fmt, ... like printf()".
> error_prepend() checks its format string argument, but
> error_propagate_prepend() does not. Fix by addint the format
> attribute to error_propagate_prepend() and error_vprepend().
> 
> This would have caught the bug fixed in the previous commit.
> 
> Missed in commit 4b5766488f "error: Fix use of error_prepend() with
> &error_fatal, &error_abort".

FWIW, I'd suggest a followup patch that adds -Wsuggest-attribute=format
to CFLAGS, as after a quick hack to try a build, I think all the things
it reports are valid cases needing the format attribute.

qemu/util/error.c:62:5: warning: function ‘error_setv’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
qemu/util/error.c:133:5: warning: function ‘error_vprepend’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
qemu/util/qemu-error.c:236:5: warning: function ‘vreport’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
qemu/contrib/libvhost-user/libvhost-user.c:161:5: warning: function ‘vu_panic’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
qemu/tools/virtiofsd/fuse_log.c:20:5: warning: function ‘default_log_func’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
qemu/tools/virtiofsd/passthrough_ll.c:2752:9: warning: function ‘log_func’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
qemu/tools/virtiofsd/passthrough_ll.c:2754:9: warning: function ‘log_func’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
qemu/hw/xen/xen-bus-helper.c:124:9: warning: function ‘xs_node_vscanf’ might be a candidate for ‘gnu_scanf’ format attribute [-Wsuggest-attribute=format]
qemu/disas.c:497:5: warning: function ‘plugin_printf’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|


Re: [PULL 4/4] qapi/error: Check format string argument in error_*prepend()
Posted by Philippe Mathieu-Daudé 5 years, 3 months ago
On 7/24/20 4:08 PM, Daniel P. Berrangé wrote:
> On Fri, Jul 24, 2020 at 03:47:04PM +0200, Markus Armbruster wrote:
>> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>> error_propagate_prepend() "behaves like error_prepend()", and
>> error_prepend() uses "formatting @fmt, ... like printf()".
>> error_prepend() checks its format string argument, but
>> error_propagate_prepend() does not. Fix by addint the format
>> attribute to error_propagate_prepend() and error_vprepend().
>>
>> This would have caught the bug fixed in the previous commit.
>>
>> Missed in commit 4b5766488f "error: Fix use of error_prepend() with
>> &error_fatal, &error_abort".
> 
> FWIW, I'd suggest a followup patch that adds -Wsuggest-attribute=format
> to CFLAGS, as after a quick hack to try a build, I think all the things
> it reports are valid cases needing the format attribute.
> 
> qemu/util/error.c:62:5: warning: function ‘error_setv’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
> qemu/util/error.c:133:5: warning: function ‘error_vprepend’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
> qemu/util/qemu-error.c:236:5: warning: function ‘vreport’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
> qemu/contrib/libvhost-user/libvhost-user.c:161:5: warning: function ‘vu_panic’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
> qemu/tools/virtiofsd/fuse_log.c:20:5: warning: function ‘default_log_func’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
> qemu/tools/virtiofsd/passthrough_ll.c:2752:9: warning: function ‘log_func’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
> qemu/tools/virtiofsd/passthrough_ll.c:2754:9: warning: function ‘log_func’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
> qemu/hw/xen/xen-bus-helper.c:124:9: warning: function ‘xs_node_vscanf’ might be a candidate for ‘gnu_scanf’ format attribute [-Wsuggest-attribute=format]
> qemu/disas.c:497:5: warning: function ‘plugin_printf’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]

I have the printf() ones ready but am waiting to be closer to 5.2.

> 
> 
> Regards,
> Daniel
>