| Message ID | 20200723091309.18690-1-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCH-for-5.1?] qapi/error: Check format string argument in error_propagate_prepend() | expand |
Am 23.07.20 um 11:13 schrieb Philippe Mathieu-Daudé: > 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 that. > > This would have catched the invalid format introduced in commit > b98e8d1230f: > > CC hw/sd/milkymist-memcard.o > hw/sd/milkymist-memcard.c: In function ‘milkymist_memcard_realize’: > hw/sd/milkymist-memcard.c:284:70: error: format ‘%s’ expects a matching ‘char *’ argument [-Werror=format=] > 284 | error_propagate_prepend(errp, err, "failed to init SD card: %s"); > | ~^ > | | > | char * > > Fixes: 4b5766488f ("Fix use of error_prepend() with &error_fatal, &error_abort") > Inspired-by: Stefan Weil <sw@weilnetz.de> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > include/qapi/error.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/qapi/error.h b/include/qapi/error.h > index 7932594dce..eeeef1a34d 100644 > --- a/include/qapi/error.h > +++ b/include/qapi/error.h > @@ -381,6 +381,7 @@ void error_propagate(Error **dst_errp, Error *local_err); > * error_propagate(dst_errp, local_err); > * Please use ERRP_GUARD() and error_prepend() instead when possible. > */ > +GCC_FMT_ATTR(3, 4) > void error_propagate_prepend(Error **dst_errp, Error *local_err, > const char *fmt, ...); > Reviewed-by: Stefan Weil <sw@weilnetz.de> error_vprepend is one more candidate for GCC_FMT_ATTR. Maybe you can add that, too. Regards, Stefan
On 7/23/20 11:44 AM, Stefan Weil wrote: > Am 23.07.20 um 11:13 schrieb Philippe Mathieu-Daudé: > >> 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 that. >> >> This would have catched the invalid format introduced in commit >> b98e8d1230f: >> >> CC hw/sd/milkymist-memcard.o >> hw/sd/milkymist-memcard.c: In function ‘milkymist_memcard_realize’: >> hw/sd/milkymist-memcard.c:284:70: error: format ‘%s’ expects a matching ‘char *’ argument [-Werror=format=] >> 284 | error_propagate_prepend(errp, err, "failed to init SD card: %s"); >> | ~^ >> | | >> | char * >> >> Fixes: 4b5766488f ("Fix use of error_prepend() with &error_fatal, &error_abort") >> Inspired-by: Stefan Weil <sw@weilnetz.de> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> include/qapi/error.h | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/include/qapi/error.h b/include/qapi/error.h >> index 7932594dce..eeeef1a34d 100644 >> --- a/include/qapi/error.h >> +++ b/include/qapi/error.h >> @@ -381,6 +381,7 @@ void error_propagate(Error **dst_errp, Error *local_err); >> * error_propagate(dst_errp, local_err); >> * Please use ERRP_GUARD() and error_prepend() instead when possible. >> */ >> +GCC_FMT_ATTR(3, 4) >> void error_propagate_prepend(Error **dst_errp, Error *local_err, >> const char *fmt, ...); >> > > > Reviewed-by: Stefan Weil <sw@weilnetz.de> > > error_vprepend is one more candidate for GCC_FMT_ATTR. Maybe you can add > that, too. This one is different as it uses a va_list. Now I realize it is only called in util/error.c, and all its callers are guarded with GCC_FMT_ATTR. Maybe we can make it static to simplify... Markus? > > Regards, > > Stefan > >
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > 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 that. > > This would have catched the invalid format introduced in commit s/catched/caught/ > b98e8d1230f: > > CC hw/sd/milkymist-memcard.o > hw/sd/milkymist-memcard.c: In function ‘milkymist_memcard_realize’: > hw/sd/milkymist-memcard.c:284:70: error: format ‘%s’ expects a matching ‘char *’ argument [-Werror=format=] > 284 | error_propagate_prepend(errp, err, "failed to init SD card: %s"); > | ~^ > | | > | char * Suggest to shorten to This would have caught the bug fixed in the previous commit. and make sure Stefan's fix actually becomes the previous commit. > Fixes: 4b5766488f ("Fix use of error_prepend() with &error_fatal, &error_abort") Kind of. Omitting the attritbute isn't wrong, just foolish / careless. I'd write Missed in commit 4b5766488f "error: Fix use of error_prepend() with &error_fatal, &error_abort". > Inspired-by: Stefan Weil <sw@weilnetz.de> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> If you like my tweaks, I can apply them in my tree. > --- > include/qapi/error.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/qapi/error.h b/include/qapi/error.h > index 7932594dce..eeeef1a34d 100644 > --- a/include/qapi/error.h > +++ b/include/qapi/error.h > @@ -381,6 +381,7 @@ void error_propagate(Error **dst_errp, Error *local_err); > * error_propagate(dst_errp, local_err); > * Please use ERRP_GUARD() and error_prepend() instead when possible. > */ > +GCC_FMT_ATTR(3, 4) > void error_propagate_prepend(Error **dst_errp, Error *local_err, > const char *fmt, ...); Reviewed-by: Markus Armbruster <armbru@redhat.com>
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > On 7/23/20 11:44 AM, Stefan Weil wrote: >> Am 23.07.20 um 11:13 schrieb Philippe Mathieu-Daudé: >> >>> 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 that. >>> >>> This would have catched the invalid format introduced in commit >>> b98e8d1230f: >>> >>> CC hw/sd/milkymist-memcard.o >>> hw/sd/milkymist-memcard.c: In function ‘milkymist_memcard_realize’: >>> hw/sd/milkymist-memcard.c:284:70: error: format ‘%s’ expects a matching ‘char *’ argument [-Werror=format=] >>> 284 | error_propagate_prepend(errp, err, "failed to init SD card: %s"); >>> | ~^ >>> | | >>> | char * >>> >>> Fixes: 4b5766488f ("Fix use of error_prepend() with &error_fatal, &error_abort") >>> Inspired-by: Stefan Weil <sw@weilnetz.de> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> --- >>> include/qapi/error.h | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/include/qapi/error.h b/include/qapi/error.h >>> index 7932594dce..eeeef1a34d 100644 >>> --- a/include/qapi/error.h >>> +++ b/include/qapi/error.h >>> @@ -381,6 +381,7 @@ void error_propagate(Error **dst_errp, Error *local_err); >>> * error_propagate(dst_errp, local_err); >>> * Please use ERRP_GUARD() and error_prepend() instead when possible. >>> */ >>> +GCC_FMT_ATTR(3, 4) >>> void error_propagate_prepend(Error **dst_errp, Error *local_err, >>> const char *fmt, ...); Wait! You put the attribute in an unusual place. We have it at the end of the declaration elsewhere in this file. Let's remain locally consistent. >> Reviewed-by: Stefan Weil <sw@weilnetz.de> >> >> error_vprepend is one more candidate for GCC_FMT_ATTR. Maybe you can add >> that, too. > > This one is different as it uses a va_list. Now I realize it is > only called in util/error.c, and all its callers are guarded with > GCC_FMT_ATTR. Maybe we can make it static to simplify... Markus? Could use GCC_FMT_ATTR(2, 0). Much less useful, but why not. Perhaps a respin would be cleaner than me applying multiple tweaks.
On 7/23/20 5:04 AM, Philippe Mathieu-Daudé wrote: >> >> error_vprepend is one more candidate for GCC_FMT_ATTR. Maybe you can add >> that, too. > > This one is different as it uses a va_list. Now I realize it is > only called in util/error.c, and all its callers are guarded with > GCC_FMT_ATTR. Maybe we can make it static to simplify... Markus? Using GCC_FMT_ATTR on va_list functions is just fine; the difference is that you spell its parameters (n, 0) instead of (n, n + 1). As for marking the function static, that was just discussed and rejected: https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg06730.html
diff --git a/include/qapi/error.h b/include/qapi/error.h index 7932594dce..eeeef1a34d 100644 --- a/include/qapi/error.h +++ b/include/qapi/error.h @@ -381,6 +381,7 @@ void error_propagate(Error **dst_errp, Error *local_err); * error_propagate(dst_errp, local_err); * Please use ERRP_GUARD() and error_prepend() instead when possible. */ +GCC_FMT_ATTR(3, 4) void error_propagate_prepend(Error **dst_errp, Error *local_err, const char *fmt, ...);
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 that. This would have catched the invalid format introduced in commit b98e8d1230f: CC hw/sd/milkymist-memcard.o hw/sd/milkymist-memcard.c: In function ‘milkymist_memcard_realize’: hw/sd/milkymist-memcard.c:284:70: error: format ‘%s’ expects a matching ‘char *’ argument [-Werror=format=] 284 | error_propagate_prepend(errp, err, "failed to init SD card: %s"); | ~^ | | | char * Fixes: 4b5766488f ("Fix use of error_prepend() with &error_fatal, &error_abort") Inspired-by: Stefan Weil <sw@weilnetz.de> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- include/qapi/error.h | 1 + 1 file changed, 1 insertion(+)