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

[Marek Vasut]

On 06/17/2016 06:24 AM, Rajesh Bhagat wrote:
> 
> 
>> -----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?

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