2014-09-15 10:47 GMT+02:00 David Laight <[email protected]>: > From: Marc Kleine-Budde [ >> On 09/15/2014 10:28 AM, David Laight wrote: >> > From: Rickard Strandqvist >> > ... >> >> Replacing strncpy with strlcpy to avoid strings that lacks null terminate. >> > ... >> >> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c >> >> b/drivers/net/can/usb/peak_usb/pcan_usb_core.c >> >> index 644e6ab..d4fe8ac 100644 >> >> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c >> >> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c >> >> @@ -830,7 +830,7 @@ static void peak_usb_disconnect(struct usb_interface >> >> *intf) >> >> char name[IFNAMSIZ]; >> >> >> >> dev->state &= ~PCAN_USB_STATE_CONNECTED; >> >> - strncpy(name, netdev->name, IFNAMSIZ); >> >> + strlcpy(name, netdev->name, IFNAMSIZ); >> >> >> >> unregister_netdev(netdev); >> >> free_candev(netdev); >> > >> > Or: >> > char name[sizeof netdev->name]; >> > memcpy(name, netdev->name, sizeof netdev->name); >> >> I would be "sizeof(foo)" in kernel coding style, > But not in mine :-) > sizeof is an operator, not a function, it's argument can be (type). > >> but let's have a look at the original code: >> >> struct net_device *netdev = dev->netdev; >> char name[IFNAMSIZ]; >> >> dev->state &= ~PCAN_USB_STATE_CONNECTED; >> strncpy(name, netdev->name, IFNAMSIZ); >> >> unregister_netdev(netdev); >> free_candev(netdev); >> >> kfree(dev->cmd_buf); >> dev->next_siblings = NULL; >> if (dev->adapter->dev_free) >> dev->adapter->dev_free(dev); >> >> dev_info(&intf->dev, "%s removed\n", name); >> >> I think it's save to use: >> >> dev_info(&intf->dev, "%s removed\n", netdev_name(dev->netdev)); >> >> instead of doing the str?cpy() in the first place. But why not use: >> >> netdev_info(dev->netdev, "removed\n"); >> >> Is the USB device information lost when using netdev_info()? > > My guess is it avoids a 'use after free' - but I'm not going to > dig that far. > > Another issue with blindly replacing strncpy() with strlcpy() > (which doesn't affect the above) is when copying status to userspace. > > David
Hi all I have been more and more careful so I did not introduce the type of error that can arise by blindly replacing strncpy to strlcpy. But this is one of the most apparent where there will not be a problem. I liked the variant: char name[sizeof(netdev->name)]; But dislike and do not understand what the point would be with memcpy variant. Kind regards Rickard Strandqvist -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
