[U-Boot] [PATCH] drivers: usb: fsl: Fix NULL terminating issue for usb controller name string

[Rajesh Bhagat]

> -----Original Message-----
> From: Marek Vasut [mailto:marex at denx.de]
> Sent: Thursday, June 02, 2016 5:49 PM
> To: Rajesh Bhagat <rajesh.bhagat at nxp.com>; u-boot at lists.denx.de
> Cc: york sun <york.sun at nxp.com>; Sriram Dash <sriram.dash at 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:marex at denx.de]
> >> Sent: Thursday, June 02, 2016 1:38 AM
> >> To: Rajesh Bhagat <rajesh.bhagat at nxp.com>; u-boot at lists.denx.de
> >> Cc: york sun <york.sun at nxp.com>; Sriram Dash <sriram.dash at 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:marex at denx.de]
> >>>> Sent: Wednesday, June 01, 2016 6:51 PM
> >>>> To: Rajesh Bhagat <rajesh.bhagat at nxp.com>; u-boot at lists.denx.de
> >>>> Cc: york sun <york.sun at nxp.com>; Sriram Dash <sriram.dash at 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.bhagat at 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