| Message ID | 20220609221702.347522-9-morbo@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clang -Wformat warning fixes | expand |
On Thu, Jun 09, 2022 at 10:16:27PM +0000, Bill Wendling wrote: > From: Bill Wendling <isanbard@gmail.com> > > When compiling with -Wformat, clang emits the following warnings: > > drivers/cdrom/cdrom.c:3454:48: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] > ret = scnprintf(info + *pos, max_size - *pos, header); > ^~~~~~ > > 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/cdrom/cdrom.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c > index 416f723a2dbb..52b40120c76e 100644 > --- a/drivers/cdrom/cdrom.c > +++ b/drivers/cdrom/cdrom.c > @@ -3451,7 +3451,7 @@ static int cdrom_print_info(const char *header, int val, char *info, > struct cdrom_device_info *cdi; > int ret; > > - ret = scnprintf(info + *pos, max_size - *pos, header); > + ret = scnprintf(info + *pos, max_size - *pos, "%s", header); > if (!ret) > return 1; > > -- > 2.36.1.255.ge46751e96f-goog > Hi Bill, Thank you for the patch, much appreciated. Looking at this though, all callers of cdrom_print_info() provide 'header' as a string literal defined within the driver, when making the call. Therefore, I'm not convinced this change is necessary for cdrom.c - that said, in this particular use case I don't think it would hurt either. I've followed the other responses on parts of this series, so I understand that a different solution is potentially in the works. Thought I'd respond anyway though out of courtesy. All the best, Phil (Uniform CDROM Maintainer)
On Sun, Jun 12, 2022 at 9:23 AM Phillip Potter <phil@philpotter.co.uk> wrote: > > On Thu, Jun 09, 2022 at 10:16:27PM +0000, Bill Wendling wrote: > > From: Bill Wendling <isanbard@gmail.com> > > > > When compiling with -Wformat, clang emits the following warnings: > > > > drivers/cdrom/cdrom.c:3454:48: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] > > ret = scnprintf(info + *pos, max_size - *pos, header); > > ^~~~~~ > > > > 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/cdrom/cdrom.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c > > index 416f723a2dbb..52b40120c76e 100644 > > --- a/drivers/cdrom/cdrom.c > > +++ b/drivers/cdrom/cdrom.c > > @@ -3451,7 +3451,7 @@ static int cdrom_print_info(const char *header, int val, char *info, > > struct cdrom_device_info *cdi; > > int ret; > > > > - ret = scnprintf(info + *pos, max_size - *pos, header); > > + ret = scnprintf(info + *pos, max_size - *pos, "%s", header); > > if (!ret) > > return 1; > > > > -- > > 2.36.1.255.ge46751e96f-goog > > > > Hi Bill, > > Thank you for the patch, much appreciated. > > Looking at this though, all callers of cdrom_print_info() provide 'header' > as a string literal defined within the driver, when making the call. > Therefore, I'm not convinced this change is necessary for cdrom.c - > that said, in this particular use case I don't think it would hurt > either. > > I've followed the other responses on parts of this series, so I > understand that a different solution is potentially in the works. > Thought I'd respond anyway though out of courtesy. > Thanks, Phillip. I pointed out in a separate response that this specific warning is disabled by default, but when I ran into while hacking stuff there weren't a lot of places where the warning popped up (at least for x86 builds) and thought it would be a nice cleanup. I understand if you don't think this patch is necessary for your code. There are some places where visual inspection of the code is "good enough" to ensure that nothing untoward will happen (Greg pointed out a similar thing in an mm/ file). Cheers! -bw
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c index 416f723a2dbb..52b40120c76e 100644 --- a/drivers/cdrom/cdrom.c +++ b/drivers/cdrom/cdrom.c @@ -3451,7 +3451,7 @@ static int cdrom_print_info(const char *header, int val, char *info, struct cdrom_device_info *cdi; int ret; - ret = scnprintf(info + *pos, max_size - *pos, header); + ret = scnprintf(info + *pos, max_size - *pos, "%s", header); if (!ret) return 1;