strncpy() is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.
Based on usage part_str usage within txgbe_read_pba_string(), we expect
part_str to be NUL-terminated but not necessarily NUL-padded:
| /* put a null character on the end of our string */
| pba_num[10] = '\0';
Interestingly, part_str is not used after txgbe_read_pba_string():
| ...
| err = txgbe_read_pba_string(wx, part_str, TXGBE_PBANUM_LENGTH);
| if (err)
| strscpy(part_str, "Unknown", sizeof(part_str));
|
| netif_info(wx, probe, netdev, "%pM\n", netdev->dev_addr);
|
| return 0;
|
|err_remove_phy:
| txgbe_remove_phy(txgbe);
|err_release_hw:
| wx_clear_interrupt_scheme(wx);
| wx_control_hw(wx, false);
|err_free_mac_table:
| kfree(wx->mac_table);
|err_pci_release_regions:
| pci_release_selected_regions(pdev,
| pci_select_bars(pdev, IORESOURCE_MEM));
|err_pci_disable_dev:
| pci_disable_device(pdev);
| return err;
|}
... this means this strncpy (or now strscpy) is probably useless. For
now, let's make the swap to strscpy() as I am not sure if this is truly
dead code or not.
Considering the above, a suitable replacement is `strscpy` [2] due to
the fact that it guarantees NUL-termination on the destination buffer
without unnecessarily NUL-padding.
Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: [email protected]
Signed-off-by: Justin Stitt <[email protected]>
---
Note: build-tested only.
Found with: $ rg "strncpy\("
---
drivers/net/ethernet/wangxun/txgbe/txgbe_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
index 5c3aed516ac2..c795fbab37be 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
@@ -739,7 +739,7 @@ static int txgbe_probe(struct pci_dev *pdev,
/* First try to read PBA as a string */
err = txgbe_read_pba_string(wx, part_str, TXGBE_PBANUM_LENGTH);
if (err)
- strncpy(part_str, "Unknown", TXGBE_PBANUM_LENGTH);
+ strscpy(part_str, "Unknown", sizeof(part_str));
netif_info(wx, probe, netdev, "%pM\n", netdev->dev_addr);
---
base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2
change-id: 20231012-strncpy-drivers-net-ethernet-wangxun-txgbe-txgbe_main-c-05152bafac20
Best regards,
--
Justin Stitt <[email protected]>
On Thu, Oct 12, 2023 at 09:20:04PM +0000, Justin Stitt wrote:
> strncpy() is deprecated for use on NUL-terminated destination strings
> [1] and as such we should prefer more robust and less ambiguous string
> interfaces.
>
> Based on usage part_str usage within txgbe_read_pba_string(), we expect
> part_str to be NUL-terminated but not necessarily NUL-padded:
> | /* put a null character on the end of our string */
> | pba_num[10] = '\0';
>
> Interestingly, part_str is not used after txgbe_read_pba_string():
> | ...
> | err = txgbe_read_pba_string(wx, part_str, TXGBE_PBANUM_LENGTH);
> | if (err)
> | strscpy(part_str, "Unknown", sizeof(part_str));
> |
> | netif_info(wx, probe, netdev, "%pM\n", netdev->dev_addr);
> |
> | return 0;
> |
> |err_remove_phy:
> | txgbe_remove_phy(txgbe);
> |err_release_hw:
> | wx_clear_interrupt_scheme(wx);
> | wx_control_hw(wx, false);
> |err_free_mac_table:
> | kfree(wx->mac_table);
> |err_pci_release_regions:
> | pci_release_selected_regions(pdev,
> | pci_select_bars(pdev, IORESOURCE_MEM));
> |err_pci_disable_dev:
> | pci_disable_device(pdev);
> | return err;
> |}
> ... this means this strncpy (or now strscpy) is probably useless. For
> now, let's make the swap to strscpy() as I am not sure if this is truly
> dead code or not.
Hi Julian
I agree, this looks like dead code.
Jiawen, please could you submit a patch cleaning this up. Either swap
to strscpy() and make use of the string, or delete it all.
Andrew
---
pw-bot: cr
On Sunday, October 15, 2023 12:31 AM, Andrew Lunn wrote:
> On Thu, Oct 12, 2023 at 09:20:04PM +0000, Justin Stitt wrote:
> > strncpy() is deprecated for use on NUL-terminated destination strings
> > [1] and as such we should prefer more robust and less ambiguous string
> > interfaces.
> >
> > Based on usage part_str usage within txgbe_read_pba_string(), we expect
> > part_str to be NUL-terminated but not necessarily NUL-padded:
> > | /* put a null character on the end of our string */
> > | pba_num[10] = '\0';
> >
> > Interestingly, part_str is not used after txgbe_read_pba_string():
> > | ...
> > | err = txgbe_read_pba_string(wx, part_str, TXGBE_PBANUM_LENGTH);
> > | if (err)
> > | strscpy(part_str, "Unknown", sizeof(part_str));
> > |
> > | netif_info(wx, probe, netdev, "%pM\n", netdev->dev_addr);
> > |
> > | return 0;
> > |
> > |err_remove_phy:
> > | txgbe_remove_phy(txgbe);
> > |err_release_hw:
> > | wx_clear_interrupt_scheme(wx);
> > | wx_control_hw(wx, false);
> > |err_free_mac_table:
> > | kfree(wx->mac_table);
> > |err_pci_release_regions:
> > | pci_release_selected_regions(pdev,
> > | pci_select_bars(pdev, IORESOURCE_MEM));
> > |err_pci_disable_dev:
> > | pci_disable_device(pdev);
> > | return err;
> > |}
> > ... this means this strncpy (or now strscpy) is probably useless. For
> > now, let's make the swap to strscpy() as I am not sure if this is truly
> > dead code or not.
>
> Hi Julian
>
> I agree, this looks like dead code.
>
> Jiawen, please could you submit a patch cleaning this up. Either swap
> to strscpy() and make use of the string, or delete it all.
Ah, I think I missed a line to print this string. I'm going to add it.