On 06/02/2016 05:14 AM, Rajesh Bhagat wrote: > > >> -----Original Message----- >> From: Marek Vasut [mailto:[email protected]] >> Sent: Thursday, June 02, 2016 1:38 AM >> To: Rajesh Bhagat <[email protected]>; [email protected] >> Cc: york sun <[email protected]>; Sriram Dash <[email protected]> >> Subject: Re: [PATCH] drivers: usb: fsl: Fix NULL terminating issue for usb >> controller >> name string >> >> On 06/01/2016 04:55 PM, Rajesh Bhagat wrote: >>> >>> >>>> -----Original Message----- >>>> From: Marek Vasut [mailto:[email protected]] >>>> Sent: Wednesday, June 01, 2016 6:51 PM >>>> To: Rajesh Bhagat <[email protected]>; [email protected] >>>> Cc: york sun <[email protected]>; Sriram Dash <[email protected]> >>>> Subject: Re: [PATCH] drivers: usb: fsl: Fix NULL terminating issue >>>> for usb controller name string >>>> >>>> On 06/01/2016 01:17 PM, Rajesh Bhagat wrote: >>>>> Fixes NULL terminating issue for usb controller name string and >>>>> performs code cleanup for intializing variables >>>>> current_usb_controller and usb_phy. >>>>> >>>>> Signed-off-by: Rajesh Bhagat <[email protected]> >>>>> --- >>>>> drivers/usb/host/ehci-fsl.c | 10 ++++------ >>>>> 1 files changed, 4 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/drivers/usb/host/ehci-fsl.c >>>>> b/drivers/usb/host/ehci-fsl.c index a43d37d..a806993 100644 >>>>> --- a/drivers/usb/host/ehci-fsl.c >>>>> +++ b/drivers/usb/host/ehci-fsl.c >>>>> @@ -49,11 +49,9 @@ int ehci_hcd_init(int index, enum usb_init_type init, >>>>> struct usb_ehci *ehci = NULL; >>>>> const char *phy_type = NULL; >>>>> size_t len; >>>>> - char current_usb_controller[5]; >>>>> + char current_usb_controller[5] = {0}; >>>>> #ifdef CONFIG_SYS_FSL_USB_INTERNAL_UTMI_PHY >>>>> - char usb_phy[5]; >>>>> - >>>>> - usb_phy[0] = '\0'; >>>>> + char usb_phy[5] = {0}; >>>>> #endif >>>>> if (has_erratum_a007075()) { >>>>> /* >>>>> @@ -64,8 +62,8 @@ int ehci_hcd_init(int index, enum usb_init_type init, >>>>> */ >>>>> mdelay(5); >>>>> } >>>>> - memset(current_usb_controller, '\0', 5); >>>>> - snprintf(current_usb_controller, 4, "usb%d", index+1); >>>>> + snprintf(current_usb_controller, sizeof(current_usb_controller), >>>>> + "usb%d", index+1); >>>> >>>> What is the actual problem here ? snprintf() will add the \0 at the >>>> end of the string, so I don't see any "null terminating issue" in the code. >>>> I can understand using the sizeof() in the snprintf(), which is valid, but >>>> that's all. >>>> >>> >>> Hello Marek, >>> >>> It is surprising for me too, but same can be verified even on x86 >>> machine using below test program, Can it be compiler optimization of memset? >> >> The man snprintf explains that: >> >> int snprintf(char *str, size_t size, const char *format, ...); >> >> The functions snprintf() and vsnprintf() write at most size bytes >> (including the >> terminating null byte ('\0')) to str. > > Hello Marek, > > You are right, snprintf does add a terminating null byte. Let me share > details of issue faced: > > It indeed is not NULL terminating issue, as snprintf adds it. This issue is > due to the length passed to > snprintf is causing the data loss of the last byte: > > Current code: snprintf(current_usb_controller, 4, "usb%d", index+1); <==== 4 > is passed > Proposed code: snprintf(current_usb_controller, 5,"usb%d", index+1); i.e. > sizeof(current_usb_controller) <== 5 is passed > > Current code is copying 4 bytes i.e. "usb1" and snprintf adds '\0' at 4th > offset which causes the data loss and final data > Becomes "usb". But proposed code is copying 5 bytes i.e. "usb1\0" and > snprintf adds '\0' at 5th offset and final data becomes > "usb1" which is the expected output.
That's correct, thus you need to use the sizeof(). -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
