diff mbox

[14/22,media] usbvision-i2c: fix format overflow warning

Message ID 20170714120720.906842-15-arnd@arndb.de (mailing list archive)
State New, archived
Headers show

Commit Message

Arnd Bergmann July 14, 2017, 12:07 p.m. UTC
gcc-7 notices that we copy a fixed length string into another
string of the same size, with additional characters:

drivers/media/usb/usbvision/usbvision-i2c.c: In function 'usbvision_i2c_register':
drivers/media/usb/usbvision/usbvision-i2c.c:190:36: error: '%d' directive writing between 1 and 11 bytes into a region of size between 0 and 47 [-Werror=format-overflow=]
  sprintf(usbvision->i2c_adap.name, "%s-%d-%s", i2c_adap_template.name,
                                    ^~~~~~~~~~
drivers/media/usb/usbvision/usbvision-i2c.c:190:2: note: 'sprintf' output between 4 and 76 bytes into a destination of size 48

We know this is fine as the template name is always "usbvision", so
we can easily avoid the warning by using this as the format string
directly.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/media/usb/usbvision/usbvision-i2c.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Hans Verkuil July 17, 2017, 12:53 p.m. UTC | #1
On 14/07/17 14:07, Arnd Bergmann wrote:
> gcc-7 notices that we copy a fixed length string into another
> string of the same size, with additional characters:
> 
> drivers/media/usb/usbvision/usbvision-i2c.c: In function 'usbvision_i2c_register':
> drivers/media/usb/usbvision/usbvision-i2c.c:190:36: error: '%d' directive writing between 1 and 11 bytes into a region of size between 0 and 47 [-Werror=format-overflow=]
>   sprintf(usbvision->i2c_adap.name, "%s-%d-%s", i2c_adap_template.name,
>                                     ^~~~~~~~~~
> drivers/media/usb/usbvision/usbvision-i2c.c:190:2: note: 'sprintf' output between 4 and 76 bytes into a destination of size 48
> 
> We know this is fine as the template name is always "usbvision", so
> we can easily avoid the warning by using this as the format string
> directly.

Hmm, how about replacing sprintf by snprintf? That feels a lot safer (this is very
old code, it's not surprising it is still using sprintf).

Regards,

	Hans

> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/media/usb/usbvision/usbvision-i2c.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/usb/usbvision/usbvision-i2c.c b/drivers/media/usb/usbvision/usbvision-i2c.c
> index fdf6b6e285da..aae9f69884da 100644
> --- a/drivers/media/usb/usbvision/usbvision-i2c.c
> +++ b/drivers/media/usb/usbvision/usbvision-i2c.c
> @@ -187,8 +187,8 @@ int usbvision_i2c_register(struct usb_usbvision *usbvision)
>  
>  	usbvision->i2c_adap = i2c_adap_template;
>  
> -	sprintf(usbvision->i2c_adap.name, "%s-%d-%s", i2c_adap_template.name,
> -		usbvision->dev->bus->busnum, usbvision->dev->devpath);
> +	sprintf(usbvision->i2c_adap.name, "usbvision-%d-%s",
> +		 usbvision->dev->bus->busnum, usbvision->dev->devpath);
>  	PDEBUG(DBG_I2C, "Adaptername: %s", usbvision->i2c_adap.name);
>  	usbvision->i2c_adap.dev.parent = &usbvision->dev->dev;
>  
>
Arnd Bergmann July 17, 2017, 12:57 p.m. UTC | #2
On Mon, Jul 17, 2017 at 2:53 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 14/07/17 14:07, Arnd Bergmann wrote:
>> gcc-7 notices that we copy a fixed length string into another
>> string of the same size, with additional characters:
>>
>> drivers/media/usb/usbvision/usbvision-i2c.c: In function 'usbvision_i2c_register':
>> drivers/media/usb/usbvision/usbvision-i2c.c:190:36: error: '%d' directive writing between 1 and 11 bytes into a region of size between 0 and 47 [-Werror=format-overflow=]
>>   sprintf(usbvision->i2c_adap.name, "%s-%d-%s", i2c_adap_template.name,
>>                                     ^~~~~~~~~~
>> drivers/media/usb/usbvision/usbvision-i2c.c:190:2: note: 'sprintf' output between 4 and 76 bytes into a destination of size 48
>>
>> We know this is fine as the template name is always "usbvision", so
>> we can easily avoid the warning by using this as the format string
>> directly.
>
> Hmm, how about replacing sprintf by snprintf? That feels a lot safer (this is very
> old code, it's not surprising it is still using sprintf).

With snprintf(), you will still get a -Wformat-truncation warning. One
of my patches
disables that warning by default, but Mauro likes build-testing with
"make W=1", so
it would still show up then.

However, we can do both: replace the string and use snprintf().

       Arnd
Hans Verkuil July 17, 2017, 12:59 p.m. UTC | #3
On 17/07/17 14:57, Arnd Bergmann wrote:
> On Mon, Jul 17, 2017 at 2:53 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On 14/07/17 14:07, Arnd Bergmann wrote:
>>> gcc-7 notices that we copy a fixed length string into another
>>> string of the same size, with additional characters:
>>>
>>> drivers/media/usb/usbvision/usbvision-i2c.c: In function 'usbvision_i2c_register':
>>> drivers/media/usb/usbvision/usbvision-i2c.c:190:36: error: '%d' directive writing between 1 and 11 bytes into a region of size between 0 and 47 [-Werror=format-overflow=]
>>>   sprintf(usbvision->i2c_adap.name, "%s-%d-%s", i2c_adap_template.name,
>>>                                     ^~~~~~~~~~
>>> drivers/media/usb/usbvision/usbvision-i2c.c:190:2: note: 'sprintf' output between 4 and 76 bytes into a destination of size 48
>>>
>>> We know this is fine as the template name is always "usbvision", so
>>> we can easily avoid the warning by using this as the format string
>>> directly.
>>
>> Hmm, how about replacing sprintf by snprintf? That feels a lot safer (this is very
>> old code, it's not surprising it is still using sprintf).
> 
> With snprintf(), you will still get a -Wformat-truncation warning. One
> of my patches
> disables that warning by default, but Mauro likes build-testing with
> "make W=1", so
> it would still show up then.
> 
> However, we can do both: replace the string and use snprintf().

Yes please!

Regards,

	Hans
diff mbox

Patch

diff --git a/drivers/media/usb/usbvision/usbvision-i2c.c b/drivers/media/usb/usbvision/usbvision-i2c.c
index fdf6b6e285da..aae9f69884da 100644
--- a/drivers/media/usb/usbvision/usbvision-i2c.c
+++ b/drivers/media/usb/usbvision/usbvision-i2c.c
@@ -187,8 +187,8 @@  int usbvision_i2c_register(struct usb_usbvision *usbvision)
 
 	usbvision->i2c_adap = i2c_adap_template;
 
-	sprintf(usbvision->i2c_adap.name, "%s-%d-%s", i2c_adap_template.name,
-		usbvision->dev->bus->busnum, usbvision->dev->devpath);
+	sprintf(usbvision->i2c_adap.name, "usbvision-%d-%s",
+		 usbvision->dev->bus->busnum, usbvision->dev->devpath);
 	PDEBUG(DBG_I2C, "Adaptername: %s", usbvision->i2c_adap.name);
 	usbvision->i2c_adap.dev.parent = &usbvision->dev->dev;