| Message ID | 20240326223825.4084412-6-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | enabled -Wformat-truncation for clang | expand |
On Tue, Mar 26, 2024 at 11:38:04PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > clang warns about printing a pair of escaped strings into a buffer that is > too short: > > drivers/platform/surface/surface3_power.c:248:3: error: 'snprintf' will always be truncated; specified size is 10, but format string expands to at least 12 [-Werror,-Wformat-truncation-non-kprintf] > 248 | snprintf(bix->serial, ARRAY_SIZE(bix->serial), "%3pE%6pE", buf + 7, buf); > | ^ > > Change the format string two print two less bytes so it always fits. The string > is still truncated, so there is no change in behavior, but the compiler no > longer warns about it. > > Fixes: 85f7582cd484 ("platform/surface: Move Surface 3 Power OpRegion driver to platform/surface") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > Not entirely sure about this one, as I've never used escaped strings, and > don't know if gcc is correct to warn here, or if the kernel defines it > differently from the standard. As far as I understand it, this is a false positive because clang does not understand the kernel's %p extensions. GCC does not warn for overflow or truncation when %p is involved but the clang developers chose to intentionally deviate from GCC in that aspect while sticking it under a separate diagnostic that we could disable. I sent a patch that did so some time ago but I guess Masahiro never applied it... https://lore.kernel.org/20231002-disable-wformat-truncation-overflow-non-kprintf-v1-1-35179205c8d9@kernel.org/ Consider dropping the changes that fix non-kprintf warnings and including that patch as part of this series. > --- > drivers/platform/surface/surface3_power.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/platform/surface/surface3_power.c b/drivers/platform/surface/surface3_power.c > index 4c0f92562a79..72f904761fde 100644 > --- a/drivers/platform/surface/surface3_power.c > +++ b/drivers/platform/surface/surface3_power.c > @@ -245,7 +245,7 @@ static int mshw0011_bix(struct mshw0011_data *cdata, struct bix *bix) > dev_err(&client->dev, "Error reading serial no: %d\n", ret); > return ret; > } else { > - snprintf(bix->serial, ARRAY_SIZE(bix->serial), "%3pE%6pE", buf + 7, buf); > + snprintf(bix->serial, ARRAY_SIZE(bix->serial), "%3pE%4pE", buf + 7, buf); > } > > /* get cycle count */ > -- > 2.39.2 >
On Wed, Mar 27, 2024 at 1:05 AM Nathan Chancellor <nathan@kernel.org> wrote: > On Tue, Mar 26, 2024 at 11:38:04PM +0100, Arnd Bergmann wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > > > clang warns about printing a pair of escaped strings into a buffer that is > > too short: > > > > drivers/platform/surface/surface3_power.c:248:3: error: 'snprintf' will always be truncated; specified size is 10, but format string expands to at least 12 [-Werror,-Wformat-truncation-non-kprintf] > > 248 | snprintf(bix->serial, ARRAY_SIZE(bix->serial), "%3pE%6pE", buf + 7, buf); > > | ^ > > > > Change the format string two print two less bytes so it always fits. The string > > is still truncated, so there is no change in behavior, but the compiler no > > longer warns about it. > > Fixes: 85f7582cd484 ("platform/surface: Move Surface 3 Power OpRegion driver to platform/surface") Hmm... Is it really a move patch (which by title should not have changed the contents)? (I haven't looked into it, though.) > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > --- > > Not entirely sure about this one, as I've never used escaped strings, and > > don't know if gcc is correct to warn here, or if the kernel defines it > > differently from the standard. > > As far as I understand it, this is a false positive because clang does > not understand the kernel's %p extensions. Yes, %pE here is special. Btw, what has already been discussed a long time is to have a validation plugin in the kernel to check those %p extensions, but IIUC nobody committed to it. That said, the patch is most likely incorrect. > GCC does not warn for > overflow or truncation when %p is involved but the clang developers > chose to intentionally deviate from GCC in that aspect while sticking it > under a separate diagnostic that we could disable. I sent a patch that > did so some time ago but I guess Masahiro never applied it... > > https://lore.kernel.org/20231002-disable-wformat-truncation-overflow-non-kprintf-v1-1-35179205c8d9@kernel.org/ > > Consider dropping the changes that fix non-kprintf warnings and > including that patch as part of this series.
Hello, just a nitpick: On Tue, Mar 26, 2024 at 11:38:04PM +0100, Arnd Bergmann wrote: > Change the format string two print two less bytes so it always fits. The string s/two/to/ > is still truncated, so there is no change in behavior, but the compiler no > longer warns about it. Best regards Uwe
diff --git a/drivers/platform/surface/surface3_power.c b/drivers/platform/surface/surface3_power.c index 4c0f92562a79..72f904761fde 100644 --- a/drivers/platform/surface/surface3_power.c +++ b/drivers/platform/surface/surface3_power.c @@ -245,7 +245,7 @@ static int mshw0011_bix(struct mshw0011_data *cdata, struct bix *bix) dev_err(&client->dev, "Error reading serial no: %d\n", ret); return ret; } else { - snprintf(bix->serial, ARRAY_SIZE(bix->serial), "%3pE%6pE", buf + 7, buf); + snprintf(bix->serial, ARRAY_SIZE(bix->serial), "%3pE%4pE", buf + 7, buf); } /* get cycle count */