-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Several new instances of -Wfortify-source after LLVM commit 0c9c9dd9a24f9 #1923
Comments
GCC appears to flag most of these... Full GCC warning instances
It does not show any warning for
so it is possible that the At the end of the day, these instances seem like they should be fixed but in my opinion, I think it is somewhat unfair that we have to do it because we see the warnings while GCC doesn't because it was implemented in |
FWIW, the documentation for some of these is under Documentation/core-api/printk-formats.rst. Let's look at one case at a time, starting with sound/aoa/soundbus/i2sbus/core.c. if (snprintf(node_name, sizeof(node_name), "%pOFn", np) != 5) so we have // clang -c -Wfortify-source tmp.c
char node_name [1];
void foo (void *n) {
__builtin_snprintf(node_name, sizeof(node_name), "%p", n);
} produces:
so clang thinks that So then it thinks that Now I'm curious why GCC doesn't warn for this; is that intentional, or a bug? Looking through some GCC commits, it doesn't seem intentional. Finally FWIW, commit be58f71 ("fortify: Add compile-time FORTIFY_SOURCE tests") seems to have added |
Filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111219 against GCC. |
in function EDIT: nevermind, is used: 1153 | static DEVICE_ATTR_RW(rebuild); |
Andrew pointed out %p omission is intentional: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78512. |
clang-18 has improved its support for detecting operations that will truncate values at runtime via -Wfortify-source. Fixes the warning: drivers/scsi/myrb.c:1906:10: warning: 'snprintf' will always be truncated; specified size is 32, but format string expands to at least 34 [-Wfortify-source] In particular, the string literal "physical device - not rebuilding\n" is indeed 34B by my count. When we have a string literal that does not contain any format flags, rather than use snprintf (sometimes with a size that's too small), let's use sprintf. This is pattern is cleaned up throughout the file. Reported-by: Nathan Chancellor <[email protected]> Closes: ClangBuiltLinux#1923 Signed-off-by: Nick Desaulniers <[email protected]>
clang-18 has improved its support for detecting operations that will truncate values at runtime via -Wfortify-source. Fixes the warning: drivers/scsi/myrs.c:1089:10: warning: 'snprintf' will always be truncated; specified size is 32, but format string expands to at least 34 [-Wfortify-source] In particular, the string literal "physical device - not rebuilding\n" is indeed 34B by my count. When we have a string literal that does not contain any format flags, rather than use snprintf (sometimes with a size that's too small), let's use sprintf. This is pattern is cleaned up throughout the file. Reported-by: Nathan Chancellor <[email protected]> Closes: ClangBuiltLinux#1923 Signed-off-by: Nick Desaulniers <[email protected]>
commit 0e90454 ("neofb: avoid overwriting fb_info fields") Maybe a question for:
|
These 2 are very clearly bugs in the source. |
I saw a fix for these fly by earlier today: https://lore.kernel.org/[email protected]/ Haven't tested it though. |
So for the non- |
Reported upstream: https://lore.kernel.org/llvm/CAKwvOdn0xoVWjQ6ufM_rojtKb0f1i1hW-J_xYGfKDNFdHwaeHQ@mail.gmail.com/ |
FYI, I submitted https://reviews.llvm.org/D159138 to address the false positive in |
* Helge Deller <[email protected]>: > On 8/29/23 18:45, Nick Desaulniers wrote: > > A recent change in clang made it better about spotting snprintf that > > will result in truncation. Nathan reported the following instances: > > > > drivers/video/fbdev/neofb.c:1959:3: warning: 'snprintf' will always be > > truncated; specified size is 16, but format string expands to at least > > 17 [-Wfortify-source] FYI, I've added the patch below to the fbdev for-next git tree. Helge From: Helge Deller <[email protected]> Subject: [PATCH] fbdev: neofb: Shorten Neomagic product name in info struct Avoid those compiler warnings: neofb.c:1959:3: warning: 'snprintf' will always be truncated; specified size is 16, but format string expands to at least 17 [-Wfortify-source] Signed-off-by: Helge Deller <[email protected]> Reported-by: Nathan Chancellor <[email protected]> Reported-by: Nick Desaulniers <[email protected]> Link: https://lore.kernel.org/all/CAKwvOdn0xoVWjQ6ufM_rojtKb0f1i1hW-J_xYGfKDNFdHwaeHQ@mail.gmail.com/ Link: ClangBuiltLinux#1923
Helge is working on a patch in: Though I had some feedback: |
Until we have a way of disabling |
* Nick Desaulniers <[email protected]>: > On Thu, Aug 31, 2023 at 2:23 PM Helge Deller <[email protected]> wrote: > > > > * Helge Deller <[email protected]>: > > > On 8/29/23 18:45, Nick Desaulniers wrote: > > > > A recent change in clang made it better about spotting snprintf that > > > > will result in truncation. Nathan reported the following instances: > > > > > > > > drivers/video/fbdev/neofb.c:1959:3: warning: 'snprintf' will always be > > > > truncated; specified size is 16, but format string expands to at least > > > > 17 [-Wfortify-source] > > > > FYI, I've added the patch below to the fbdev for-next git tree. > > [...] > > This indeed makes the warning go away, but that's more so due to the > use of strscpy now rather than snprintf. That alone is a good change > but we still have definite truncation. See below: > [...] Nick, thanks for your review and findings! Now every string should be max. 15 chars (which fits with the trailing NUL into the char[16] array). Helge Subject: [PATCH] fbdev: neofb: Shorten Neomagic product name in info struct Avoid those compiler warnings: neofb.c:1959:3: warning: 'snprintf' will always be truncated; specified size is 16, but format string expands to at least 17 [-Wfortify-source] Signed-off-by: Helge Deller <[email protected]> Reported-by: Nathan Chancellor <[email protected]> Reported-by: Nick Desaulniers <[email protected]> Link: https://lore.kernel.org/all/CAKwvOdn0xoVWjQ6ufM_rojtKb0f1i1hW-J_xYGfKDNFdHwaeHQ@mail.gmail.com/ Link: ClangBuiltLinux#1923
Avoid those compiler warnings: neofb.c:1959:3: warning: 'snprintf' will always be truncated; specified size is 16, but format string expands to at least 17 [-Wfortify-source] Signed-off-by: Helge Deller <[email protected]> Reported-by: Nathan Chancellor <[email protected]> Reported-by: Nick Desaulniers <[email protected]> Reviewed-by: Nick Desaulniers <[email protected]> Link: https://lore.kernel.org/all/CAKwvOdn0xoVWjQ6ufM_rojtKb0f1i1hW-J_xYGfKDNFdHwaeHQ@mail.gmail.com/ Link: ClangBuiltLinux#1923
commit f7cf224 upstream. Building dasd_eckd.o with latest clang reveals this bug: CC drivers/s390/block/dasd_eckd.o drivers/s390/block/dasd_eckd.c:1082:3: warning: 'snprintf' will always be truncated; specified size is 1, but format string expands to at least 11 [-Wfortify-source] 1082 | snprintf(print_uid, sizeof(*print_uid), | ^ drivers/s390/block/dasd_eckd.c:1087:3: warning: 'snprintf' will always be truncated; specified size is 1, but format string expands to at least 10 [-Wfortify-source] 1087 | snprintf(print_uid, sizeof(*print_uid), | ^ Fix this by moving and using the existing UID_STRLEN for the arrays that are being written to. Also rename UID_STRLEN to DASD_UID_STRLEN to clarify its scope. Fixes: 2359696 ("s390/dasd: split up dasd_eckd_read_conf") Reviewed-by: Peter Oberparleiter <[email protected]> Signed-off-by: Heiko Carstens <[email protected]> Tested-by: Nick Desaulniers <[email protected]> # build Reported-by: Nathan Chancellor <[email protected]> Closes: ClangBuiltLinux#1923 Reviewed-by: Nick Desaulniers <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Jens Axboe <[email protected]> Signed-off-by: Greg Kroah-Hartman <[email protected]>
commit f7cf224 upstream. Building dasd_eckd.o with latest clang reveals this bug: CC drivers/s390/block/dasd_eckd.o drivers/s390/block/dasd_eckd.c:1082:3: warning: 'snprintf' will always be truncated; specified size is 1, but format string expands to at least 11 [-Wfortify-source] 1082 | snprintf(print_uid, sizeof(*print_uid), | ^ drivers/s390/block/dasd_eckd.c:1087:3: warning: 'snprintf' will always be truncated; specified size is 1, but format string expands to at least 10 [-Wfortify-source] 1087 | snprintf(print_uid, sizeof(*print_uid), | ^ Fix this by moving and using the existing UID_STRLEN for the arrays that are being written to. Also rename UID_STRLEN to DASD_UID_STRLEN to clarify its scope. Fixes: 2359696 ("s390/dasd: split up dasd_eckd_read_conf") Reviewed-by: Peter Oberparleiter <[email protected]> Signed-off-by: Heiko Carstens <[email protected]> Tested-by: Nick Desaulniers <[email protected]> # build Reported-by: Nathan Chancellor <[email protected]> Closes: ClangBuiltLinux#1923 Reviewed-by: Nick Desaulniers <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Jens Axboe <[email protected]> Signed-off-by: Greg Kroah-Hartman <[email protected]>
commit f7cf224 upstream. Building dasd_eckd.o with latest clang reveals this bug: CC drivers/s390/block/dasd_eckd.o drivers/s390/block/dasd_eckd.c:1082:3: warning: 'snprintf' will always be truncated; specified size is 1, but format string expands to at least 11 [-Wfortify-source] 1082 | snprintf(print_uid, sizeof(*print_uid), | ^ drivers/s390/block/dasd_eckd.c:1087:3: warning: 'snprintf' will always be truncated; specified size is 1, but format string expands to at least 10 [-Wfortify-source] 1087 | snprintf(print_uid, sizeof(*print_uid), | ^ Fix this by moving and using the existing UID_STRLEN for the arrays that are being written to. Also rename UID_STRLEN to DASD_UID_STRLEN to clarify its scope. Fixes: 2359696 ("s390/dasd: split up dasd_eckd_read_conf") Reviewed-by: Peter Oberparleiter <[email protected]> Signed-off-by: Heiko Carstens <[email protected]> Tested-by: Nick Desaulniers <[email protected]> # build Reported-by: Nathan Chancellor <[email protected]> Closes: ClangBuiltLinux#1923 Reviewed-by: Nick Desaulniers <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Jens Axboe <[email protected]> Signed-off-by: Greg Kroah-Hartman <[email protected]>
commit f7cf224 upstream. Building dasd_eckd.o with latest clang reveals this bug: CC drivers/s390/block/dasd_eckd.o drivers/s390/block/dasd_eckd.c:1082:3: warning: 'snprintf' will always be truncated; specified size is 1, but format string expands to at least 11 [-Wfortify-source] 1082 | snprintf(print_uid, sizeof(*print_uid), | ^ drivers/s390/block/dasd_eckd.c:1087:3: warning: 'snprintf' will always be truncated; specified size is 1, but format string expands to at least 10 [-Wfortify-source] 1087 | snprintf(print_uid, sizeof(*print_uid), | ^ Fix this by moving and using the existing UID_STRLEN for the arrays that are being written to. Also rename UID_STRLEN to DASD_UID_STRLEN to clarify its scope. Fixes: 2359696 ("s390/dasd: split up dasd_eckd_read_conf") Reviewed-by: Peter Oberparleiter <[email protected]> Signed-off-by: Heiko Carstens <[email protected]> Tested-by: Nick Desaulniers <[email protected]> # build Reported-by: Nathan Chancellor <[email protected]> Closes: ClangBuiltLinux#1923 Reviewed-by: Nick Desaulniers <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Jens Axboe <[email protected]> Signed-off-by: Greg Kroah-Hartman <[email protected]>
… alternative form The wrong handling of %x specifier with alternative form causes a false positive in linux kernel (ClangBuiltLinux/linux#1923 (comment)) The kernel code: https://github.com/torvalds/linux/blob/651a00bc56403161351090a9d7ddbd7095975324/drivers/media/pci/cx18/cx18-mailbox.c#L99 This patch fixes this handling, and also adds some standard wordings as comments to clarify the reason. Reviewed By: nickdesaulniers Differential Revision: https://reviews.llvm.org/D159138
@hazohelet has fixed the false-positive related to %#o, %#x, and %#X (drivers/media/pci/cx18/cx18-mailbox.c) and has a patch related to %n in llvm/llvm-project#65969. Richard asked that this be split out into it's own flag so we can disable it (-Wno-fortify-source-percent-p or -Wno-fortify-source-gcc-compat or some such). |
Maybe not but won't instances of |
Yes though -Wformat-overflow is pretty rare. |
I'll send whatever diff you are willing to sign off on :) we can either turn off both now or we can turn off truncation now and overflow when it matters, I don't personally have much of a preference. I'll wait to send the change until the LLVM PR lands and we are pretty confident it won't get reverted, just to make sure we don't have dead code in the kernel. |
commit f7cf224 upstream. Building dasd_eckd.o with latest clang reveals this bug: CC drivers/s390/block/dasd_eckd.o drivers/s390/block/dasd_eckd.c:1082:3: warning: 'snprintf' will always be truncated; specified size is 1, but format string expands to at least 11 [-Wfortify-source] 1082 | snprintf(print_uid, sizeof(*print_uid), | ^ drivers/s390/block/dasd_eckd.c:1087:3: warning: 'snprintf' will always be truncated; specified size is 1, but format string expands to at least 10 [-Wfortify-source] 1087 | snprintf(print_uid, sizeof(*print_uid), | ^ Fix this by moving and using the existing UID_STRLEN for the arrays that are being written to. Also rename UID_STRLEN to DASD_UID_STRLEN to clarify its scope. Fixes: 2359696 ("s390/dasd: split up dasd_eckd_read_conf") Reviewed-by: Peter Oberparleiter <[email protected]> Signed-off-by: Heiko Carstens <[email protected]> Tested-by: Nick Desaulniers <[email protected]> # build Reported-by: Nathan Chancellor <[email protected]> Closes: ClangBuiltLinux/linux#1923 Reviewed-by: Nick Desaulniers <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Jens Axboe <[email protected]> Signed-off-by: Greg Kroah-Hartman <[email protected]>
… alternative form The wrong handling of %x specifier with alternative form causes a false positive in linux kernel (ClangBuiltLinux/linux#1923 (comment)) The kernel code: https://github.com/torvalds/linux/blob/651a00bc56403161351090a9d7ddbd7095975324/drivers/media/pci/cx18/cx18-mailbox.c#L99 This patch fixes this handling, and also adds some standard wordings as comments to clarify the reason. Reviewed By: nickdesaulniers Differential Revision: https://reviews.llvm.org/D159138
I suspect clang-18 is now fixed (but haven't yet confirmed the logs) |
commit f7cf22424665043787a96a66a048ff6b2cfd473c upstream. Building dasd_eckd.o with latest clang reveals this bug: CC drivers/s390/block/dasd_eckd.o drivers/s390/block/dasd_eckd.c:1082:3: warning: 'snprintf' will always be truncated; specified size is 1, but format string expands to at least 11 [-Wfortify-source] 1082 | snprintf(print_uid, sizeof(*print_uid), | ^ drivers/s390/block/dasd_eckd.c:1087:3: warning: 'snprintf' will always be truncated; specified size is 1, but format string expands to at least 10 [-Wfortify-source] 1087 | snprintf(print_uid, sizeof(*print_uid), | ^ Fix this by moving and using the existing UID_STRLEN for the arrays that are being written to. Also rename UID_STRLEN to DASD_UID_STRLEN to clarify its scope. Fixes: 23596961b437 ("s390/dasd: split up dasd_eckd_read_conf") Reviewed-by: Peter Oberparleiter <[email protected]> Signed-off-by: Heiko Carstens <[email protected]> Tested-by: Nick Desaulniers <[email protected]> # build Reported-by: Nathan Chancellor <[email protected]> Closes: ClangBuiltLinux/linux#1923 Reviewed-by: Nick Desaulniers <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Jens Axboe <[email protected]> Signed-off-by: Greg Kroah-Hartman <[email protected]>
Even with llvm/llvm-project@56c3b8e, we still have numerous instances of @nathanchance next week can you test + send #1923 (comment) ? |
Yes, I'll add it to my TODO but if we are still seeing warnings even with that LLVM change merged, something has gone wrong with that change. We shouldn't be seeing these instances due to |
It looks like CI has picked up llvm/llvm-project@56c3b8e, as mainline clang-18 is now green: https://github.com/ClangBuiltLinux/continuous-integration2/actions/runs/6380048759 |
Recently, clang added support for -Wformat-overflow and -Wformat-truncation. When building the kernel, it was discovered that clang's implementation of these warnings handles the '%p' specifier, which differs from GCC's implementation. This results in false positive warnings due to the kernel's various '%p' extensions. Fortunately, the clang developers placed this warning difference into a separate flag, allowing the kernel to turn off the warning for '%p' unconditionally. This is not currently an issue for a normal build, as -Wformat-overflow and -Wformat-truncation are unconditionally disabled, which includes this sub-warning. However, ever since commit 6d4ab2e ("extrawarn: enable format and stringop overflow warnings in W=1"), these warnings are in W=1 and the goal is to enable them in the normal build once they are all eliminated. Disable the warnings for W=1 to avoid false positives. This block should move with -Wformat-overflow and -Wformat-truncation when they are enabled for a normal build. Link: ClangBuiltLinux#1923 Link: llvm/llvm-project#64871 Link: llvm/llvm-project#65969 Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111219 Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78512 Signed-off-by: Nathan Chancellor <[email protected]>
BugLink: https://bugs.launchpad.net/bugs/2035588 commit f7cf224 upstream. Building dasd_eckd.o with latest clang reveals this bug: CC drivers/s390/block/dasd_eckd.o drivers/s390/block/dasd_eckd.c:1082:3: warning: 'snprintf' will always be truncated; specified size is 1, but format string expands to at least 11 [-Wfortify-source] 1082 | snprintf(print_uid, sizeof(*print_uid), | ^ drivers/s390/block/dasd_eckd.c:1087:3: warning: 'snprintf' will always be truncated; specified size is 1, but format string expands to at least 10 [-Wfortify-source] 1087 | snprintf(print_uid, sizeof(*print_uid), | ^ Fix this by moving and using the existing UID_STRLEN for the arrays that are being written to. Also rename UID_STRLEN to DASD_UID_STRLEN to clarify its scope. Fixes: 2359696 ("s390/dasd: split up dasd_eckd_read_conf") Reviewed-by: Peter Oberparleiter <[email protected]> Signed-off-by: Heiko Carstens <[email protected]> Tested-by: Nick Desaulniers <[email protected]> # build Reported-by: Nathan Chancellor <[email protected]> Closes: ClangBuiltLinux#1923 Reviewed-by: Nick Desaulniers <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Jens Axboe <[email protected]> Signed-off-by: Greg Kroah-Hartman <[email protected]> Signed-off-by: Andrea Righi <[email protected]>
BugLink: https://bugs.launchpad.net/bugs/2042884 commit f7cf22424665043787a96a66a048ff6b2cfd473c upstream. Building dasd_eckd.o with latest clang reveals this bug: CC drivers/s390/block/dasd_eckd.o drivers/s390/block/dasd_eckd.c:1082:3: warning: 'snprintf' will always be truncated; specified size is 1, but format string expands to at least 11 [-Wfortify-source] 1082 | snprintf(print_uid, sizeof(*print_uid), | ^ drivers/s390/block/dasd_eckd.c:1087:3: warning: 'snprintf' will always be truncated; specified size is 1, but format string expands to at least 10 [-Wfortify-source] 1087 | snprintf(print_uid, sizeof(*print_uid), | ^ Fix this by moving and using the existing UID_STRLEN for the arrays that are being written to. Also rename UID_STRLEN to DASD_UID_STRLEN to clarify its scope. Fixes: 2359696 ("s390/dasd: split up dasd_eckd_read_conf") Reviewed-by: Peter Oberparleiter <[email protected]> Signed-off-by: Heiko Carstens <[email protected]> Tested-by: Nick Desaulniers <[email protected]> # build Reported-by: Nathan Chancellor <[email protected]> Closes: ClangBuiltLinux/linux#1923 Reviewed-by: Nick Desaulniers <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Jens Axboe <[email protected]> Signed-off-by: Greg Kroah-Hartman <[email protected]> Signed-off-by: Kamal Mostafa <[email protected]> Signed-off-by: Stefan Bader <[email protected]>
BugLink: https://bugs.launchpad.net/bugs/2042884 commit f7cf22424665043787a96a66a048ff6b2cfd473c upstream. Building dasd_eckd.o with latest clang reveals this bug: CC drivers/s390/block/dasd_eckd.o drivers/s390/block/dasd_eckd.c:1082:3: warning: 'snprintf' will always be truncated; specified size is 1, but format string expands to at least 11 [-Wfortify-source] 1082 | snprintf(print_uid, sizeof(*print_uid), | ^ drivers/s390/block/dasd_eckd.c:1087:3: warning: 'snprintf' will always be truncated; specified size is 1, but format string expands to at least 10 [-Wfortify-source] 1087 | snprintf(print_uid, sizeof(*print_uid), | ^ Fix this by moving and using the existing UID_STRLEN for the arrays that are being written to. Also rename UID_STRLEN to DASD_UID_STRLEN to clarify its scope. Fixes: 2359696 ("s390/dasd: split up dasd_eckd_read_conf") Reviewed-by: Peter Oberparleiter <[email protected]> Signed-off-by: Heiko Carstens <[email protected]> Tested-by: Nick Desaulniers <[email protected]> # build Reported-by: Nathan Chancellor <[email protected]> Closes: ClangBuiltLinux/linux#1923 Reviewed-by: Nick Desaulniers <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Jens Axboe <[email protected]> Signed-off-by: Greg Kroah-Hartman <[email protected]> Signed-off-by: Kamal Mostafa <[email protected]> Signed-off-by: Stefan Bader <[email protected]>
This should be all squared away as of commit 908dd508276d ("kbuild: enable -Wformat-truncation on clang") in 6.10. |
After llvm/llvm-project@0c9c9dd, I see several new instances of
-Wfortify-source
(LKFT reported one as well):Not too many but for at least the
drivers/video/fbdev
ones, the size is in a UAPI header (include/uapi/linux/fb.h
), so I am not really sure how we will go about fixing these...I have not checked to see if GCC warns about these same instances but
-Wformat-truncation
is disabled in mainline and placed inW=1
in -next but we don't get that same escape hatch because this is not a separate warning option.The text was updated successfully, but these errors were encountered: