> -----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? Best Regards, Rajesh Bhagat > -- > Best regards, > Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
