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

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

[Rajesh Bhagat]

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);
 
 	switch (index) {
 	case 0:
-- 
1.7.7.4

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

[Marek Vasut]

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.

>  	switch (index) {
>  	case 0:
> 


-- 
Best regards,
Marek Vasut

[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: 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?

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

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

[Marek Vasut]

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

[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 1:38 AM
> 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 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.

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'@5th offset and final data becomes
"usb1" which is the expected output.

Best Regards,
Rajesh Bhagat 

> 
> > 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

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

[Marek Vasut]

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@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 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.
> 
> 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.

That's correct, thus you need to use the sizeof().

-- 
Best regards,
Marek Vasut

[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@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/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@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 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.
> >
> > 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] [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@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/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@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 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.
>>>
>>> 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