mbox series

[0/2] Fix uninit and signed integer overflow errors

Message ID cover.1667480280.git.skhan@linuxfoundation.org
Headers show
Series Fix uninit and signed integer overflow errors | expand

Message

Shuah Khan Nov. 3, 2022, 1:12 p.m. UTC
Fix uninitialized variable and signed integer overflow errors reported
by cppcheck.

Shuah Khan (2):
  usb/usbip: fix uninitialized variables errors
  usb/usbip: Fix v_recv_cmd_submit() to use PIPE_BULK define

 drivers/usb/usbip/stub_main.c     | 2 +-
 drivers/usb/usbip/stub_rx.c       | 4 ++--
 drivers/usb/usbip/stub_tx.c       | 4 ++--
 drivers/usb/usbip/usbip_event.c   | 2 +-
 drivers/usb/usbip/vhci_hcd.c      | 2 +-
 drivers/usb/usbip/vhci_rx.c       | 2 +-
 drivers/usb/usbip/vhci_tx.c       | 4 ++--
 drivers/usb/usbip/vudc_dev.c      | 2 +-
 drivers/usb/usbip/vudc_rx.c       | 7 +++++--
 drivers/usb/usbip/vudc_transfer.c | 4 ++--
 10 files changed, 18 insertions(+), 15 deletions(-)

Comments

Greg Kroah-Hartman Nov. 3, 2022, 1:21 p.m. UTC | #1
On Thu, Nov 03, 2022 at 07:12:42AM -0600, Shuah Khan wrote:
> Fix uninitialized variable errors reported by cppcheck. One example
> below.
> 
> usbip/stub_main.c:284:10: error: Uninitialized variables: priv.seqnum, priv.sdev, priv.urbs, priv.sgl, priv.num_urbs, priv.completed_urbs, priv.urb_status, priv.unlinking [uninitvar]
>   return priv;
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> ---
>  drivers/usb/usbip/stub_main.c     | 2 +-
>  drivers/usb/usbip/stub_rx.c       | 4 ++--
>  drivers/usb/usbip/stub_tx.c       | 4 ++--
>  drivers/usb/usbip/usbip_event.c   | 2 +-
>  drivers/usb/usbip/vhci_hcd.c      | 2 +-
>  drivers/usb/usbip/vhci_rx.c       | 2 +-
>  drivers/usb/usbip/vhci_tx.c       | 4 ++--
>  drivers/usb/usbip/vudc_dev.c      | 2 +-
>  drivers/usb/usbip/vudc_rx.c       | 2 +-
>  drivers/usb/usbip/vudc_transfer.c | 4 ++--
>  10 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/usbip/stub_main.c b/drivers/usb/usbip/stub_main.c
> index e8c3131a8543..e1248b971218 100644
> --- a/drivers/usb/usbip/stub_main.c
> +++ b/drivers/usb/usbip/stub_main.c
> @@ -277,7 +277,7 @@ static DRIVER_ATTR_WO(rebind);
>  
>  static struct stub_priv *stub_priv_pop_from_listhead(struct list_head *listhead)
>  {
> -	struct stub_priv *priv, *tmp;
> +	struct stub_priv *priv = NULL, *tmp;
>  
>  	list_for_each_entry_safe(priv, tmp, listhead, list) {

cppcheck is wrong here, the code is fine, and setting priv to NULL does
nothing.  If it was required, gcc would have hopefully caught it, and
the code would have never worked :)

So are you sure all of these changes are really needed?  Last time I
looked, cppcheck wasn't all that smart when it came to the kernel and
threw up huge numbers of false-positives, like this one.

thanks,

greg k-h
Shuah Khan Nov. 4, 2022, 3:18 p.m. UTC | #2
On 11/3/22 07:21, Greg KH wrote:
> On Thu, Nov 03, 2022 at 07:12:42AM -0600, Shuah Khan wrote:
>> Fix uninitialized variable errors reported by cppcheck. One example
>> below.
>>
>> usbip/stub_main.c:284:10: error: Uninitialized variables: priv.seqnum, priv.sdev, priv.urbs, priv.sgl, priv.num_urbs, priv.completed_urbs, priv.urb_status, priv.unlinking [uninitvar]
>>    return priv;
>>
>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>> ---
>>   drivers/usb/usbip/stub_main.c     | 2 +-
>>   drivers/usb/usbip/stub_rx.c       | 4 ++--
>>   drivers/usb/usbip/stub_tx.c       | 4 ++--
>>   drivers/usb/usbip/usbip_event.c   | 2 +-
>>   drivers/usb/usbip/vhci_hcd.c      | 2 +-
>>   drivers/usb/usbip/vhci_rx.c       | 2 +-
>>   drivers/usb/usbip/vhci_tx.c       | 4 ++--
>>   drivers/usb/usbip/vudc_dev.c      | 2 +-
>>   drivers/usb/usbip/vudc_rx.c       | 2 +-
>>   drivers/usb/usbip/vudc_transfer.c | 4 ++--
>>   10 files changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/usb/usbip/stub_main.c b/drivers/usb/usbip/stub_main.c
>> index e8c3131a8543..e1248b971218 100644
>> --- a/drivers/usb/usbip/stub_main.c
>> +++ b/drivers/usb/usbip/stub_main.c
>> @@ -277,7 +277,7 @@ static DRIVER_ATTR_WO(rebind);
>>   
>>   static struct stub_priv *stub_priv_pop_from_listhead(struct list_head *listhead)
>>   {
>> -	struct stub_priv *priv, *tmp;
>> +	struct stub_priv *priv = NULL, *tmp;
>>   
>>   	list_for_each_entry_safe(priv, tmp, listhead, list) {
> 
> cppcheck is wrong here, the code is fine, and setting priv to NULL does
> nothing.  If it was required, gcc would have hopefully caught it, and
> the code would have never worked :)
> 
> So are you sure all of these changes are really needed?  Last time I
> looked, cppcheck wasn't all that smart when it came to the kernel and
> threw up huge numbers of false-positives, like this one.
> 

You are right that this one is a false positive. I will take a close look at
the others in this patch.

thanks,
-- Shuah