GNOME Bugzilla – Bug 725993
_clutter_debug_messagev() uses non-literal format with -Werror=format-nonliteral
Last modified: 2021-06-10 11:31:08 UTC
Clutter is currently broken when using clang with the default CFLAGS. _clutter_debug_messagev() is a wrapper around g_logv() which adds timestamp information. It pastes together a format string dynamically in order to do so, but clutter has -Werror=format-nonliteral in its CFLAGS. It seems that gcc doesn't enforce -Werror=format-nonliteral with va_list whereas clang does (by ensuring that the input format has come from a place where it is appropriately tagged). In most cases it's possible to fix this by annotating the function so that the format string checks will be passed up to the next level (and this should be done anyway -- these checks are currently missing on users of _clutter_debug_message(). In this case, due to the dynamic construction of the format string, this is not possible. In fact, there may be an actual issue here (although extremely theoretical): if you're in some bizarre locale and the formatting of the timestamp results in a '%' being used as a separator or something, this will become part of the format string... A solution is to paste the results together after formatting instead of before, but this introduces an extra allocation (and there is no g_log_literal() to work around that). A workaround (that we have been using) is to set -Wno-error=format-nonliteral in the CFLAGS.
as an aside: I would not be totally against adding timestamps to g_logv()...
I'm afraid I don't see the issue: the timestamp is G_GINT64_FORMAT, and the result is concatenated to the format string after it has been resolved — i.e. the timestamp cannot contain stray '%', unless G_GINT64_FORMAT gets redefined. I'm amenable to adding G_GNUC_PRINTF() to _clutter_debug_message(); I'm also amendable to add timestamps to g_logv() in the same way I added them to clutter_debug_message(); I'd like to understand why you think the timestamp is a format string prior to concatenation.
Created attachment 271560 [details] [review] debug: Add G_GNUC_PRINTF annotation to clutter_debug_message* This allows us to catch eventual format string issues. The patch already triggered a format string check error in clutter-stage.c
(In reply to comment #2) > I'd like to understand why you think the timestamp is > a format string prior to concatenation. You're including the timestamp as part of the format string. It is definitely part of the format string. I agree that it would not contain % under any plausible scenario. I was speculating that some bizarre locale-dependent conversion of integers might cause it to do that, but no such locale exists, and such a conversion may be in violation of POSIX anyway (which may specify that %d is always rendered in the C-locale way). The problem here is pretty simple, though: you're using -Werror=format-nonliteral and then you're using a non-literal format string. The only time you can have an exception to this rule is when the format string has come in as an argument to a function that has been tagged as a printf-like function. Since you take that format string and perform additional manipulations on it, the compiler has no way to know. The reason you don't see this issue is because the meaning of this flag is a bit different under gcc and clang. In gcc, functions that take 'va_list' instead of '...' get an exception. In clang, they don't.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version of clutter, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a ticket at https://gitlab.gnome.org/GNOME/clutter/-/issues/ Thank you for your understanding and your help.