| Message ID | 20220609221702.347522-1-morbo@google.com |
|---|---|
| Headers | show |
| Series | Clang -Wformat warning fixes | expand |
From: Jan Engelhardt > Sent: 10 June 2022 09:32 > > > On Friday 2022-06-10 10:17, David Laight wrote: > >> > >> Calling a "printf" style function is already insanely expensive. :-) I > >> understand that it's not okay blithely to increase runtime resources > >> simply because it's already slow, but in this case it's worthwhile. > > > >Yep, IMHO definitely should be fixed. > >It is even possible that using "%s" is faster because the printf > >code doesn't have to scan the string for format effectors. > > I see no special handling; the vsnprintf function just loops > over fmt as usual and I see no special casing of fmt by > e.g. strcmp(fmt, "%s") == 0 to take a shortcut. Consider the difference between: printf("fubar"); and printf("%s", "fubar"); In the former all of "fubar" is checked for '%'. In the latter only the length of "fubar" has to be counted. FWIW the patch description should probably by: use "%s" when formatting a single string. (or something to that effect). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Thu, Jun 9, 2022 at 10:18 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Thu, Jun 09, 2022 at 10:16:26PM +0000, Bill Wendling wrote: > > From: Bill Wendling <isanbard@gmail.com> > > Why isn't that matching your From: line in the email? > There must be something wrong with my .gitconfig file. I"ll check into it. > > > > When compiling with -Wformat, clang emits the following warnings: > > Is that ever a default build option for the kernel? > We want to enable -Wformat for clang. I believe that these specific warnings have been disabled, but I'm confused as to why, because they're valid warnings. When I compiled with the warning enabled, there were only a few (12) places that needed changes, so thought that patches would be a nice cleanup, even though the warning itself is disabled. > > drivers/char/mem.c:775:16: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] > > NULL, devlist[minor].name); > > ^~~~~~~~~~~~~~~~~~~ > > > > Use a string literal for the format string. > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/378 > > Signed-off-by: Bill Wendling <isanbard@gmail.com> > > --- > > drivers/char/mem.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/char/mem.c b/drivers/char/mem.c > > index 84ca98ed1dad..32d821ba9e4d 100644 > > --- a/drivers/char/mem.c > > +++ b/drivers/char/mem.c > > @@ -772,7 +772,7 @@ static int __init chr_dev_init(void) > > continue; > > > > device_create(mem_class, NULL, MKDEV(MEM_MAJOR, minor), > > - NULL, devlist[minor].name); > > + NULL, "%s", devlist[minor].name); > > Please explain how this static string can ever be user controlled. > All someone would need to do is accidentally insert an errant '%' in one of the strings for this function call to perform unexpected actions---at the very least reading memory that's not allocated and may contain garbage, thereby decreasing performance and possibly overrunning some buffer. Perhaps in this specific scenario it's unlikely, but "device_create()" is used in a lot more places than here. This patch is a general code cleanup. -bw