On 06/17/2016 06:24 AM, Rajesh Bhagat wrote: > > >> -----Original Message----- >> From: Marek Vasut [mailto:ma...@denx.de] >> Sent: Thursday, June 02, 2016 5:49 PM >> To: Rajesh Bhagat <rajesh.bha...@nxp.com>; u-boot@lists.denx.de >> Cc: york sun <york....@nxp.com>; Sriram Dash <sriram.d...@nxp.com> >> Subject: Re: [PATCH] drivers: usb: fsl: Fix NULL terminating issue for usb >> controller >> name string >> >> On 06/02/2016 05:14 AM, Rajesh Bhagat wrote: >>> >>> >>>> -----Original Message----- >>>> From: Marek Vasut [mailto:ma...@denx.de] >>>> Sent: Thursday, June 02, 2016 1:38 AM >>>> To: Rajesh Bhagat <rajesh.bha...@nxp.com>; u-boot@lists.denx.de >>>> Cc: york sun <york....@nxp.com>; Sriram Dash <sriram.d...@nxp.com> >>>> 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:ma...@denx.de] >>>>>> Sent: Wednesday, June 01, 2016 6:51 PM >>>>>> To: Rajesh Bhagat <rajesh.bha...@nxp.com>; u-boot@lists.denx.de >>>>>> Cc: york sun <york....@nxp.com>; Sriram Dash <sriram.d...@nxp.com> >>>>>> 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 <rajesh.bha...@nxp.com> >>>>>>> --- >>>>>>> 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. > > Hello Marek, > >> >> That's correct, thus you need to use the sizeof(). >> > > Are you planning to pick this up?
No, I expect a 1-liner V2 patch changing just the 4 to sizeof() in the snprintf() argument. The rest of the patch is unrelated. -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
