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

[Marek Vasut <marex@denx.de>]

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@nxp.com>; u-boot at lists.denx.de
>> Cc: york sun <york.sun@nxp.com>; Sriram Dash <sriram.dash@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@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.

> Output #1 : current_usb_controller usb
> Output #2 : current_usb_controller usb1 (Expected Output)
> 
> int main()
> {
>         int index = 0;
> #if 1  
>         char current_usb_controller[5];
>         memset(current_usb_controller, '\0', 5);                        
>         snprintf(current_usb_controller, 4, "usb%d", index+1);
> #else
>         char current_usb_controller[5] = {0};
>         snprintf(current_usb_controller, sizeof(current_usb_controller), "usb%d", index+1);
> #endif
> 
>         printf("current_usb_controller %s\n", current_usb_controller);
>         return 0;
> }
> 
> Best Regards,
> Rajesh Bhagat 
> 
>>>  	switch (index) {
>>>  	case 0:
>>>
>>
>>
>> --
>> Best regards,
>> Marek Vasut


-- 
Best regards,
Marek Vasut