| Message ID | 1464779851-29744-1-git-send-email-rajesh.bhagat@nxp.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
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: >
> -----Original Message----- > From: Marek Vasut [mailto:marex@denx.de] > Sent: Wednesday, June 01, 2016 6:51 PM > To: Rajesh Bhagat <rajesh.bhagat@nxp.com>; u-boot@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
On 06/01/2016 04:55 PM, Rajesh Bhagat wrote: > > >> -----Original Message----- >> From: Marek Vasut [mailto:marex@denx.de] >> Sent: Wednesday, June 01, 2016 6:51 PM >> To: Rajesh Bhagat <rajesh.bhagat@nxp.com>; u-boot@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
> -----Original Message----- > From: Marek Vasut [mailto:marex@denx.de] > Sent: Thursday, June 02, 2016 1:38 AM > To: Rajesh Bhagat <rajesh.bhagat@nxp.com>; u-boot@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@denx.de] > >> Sent: Wednesday, June 01, 2016 6:51 PM > >> To: Rajesh Bhagat <rajesh.bhagat@nxp.com>; u-boot@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. 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
On 06/02/2016 05:14 AM, Rajesh Bhagat wrote: > > >> -----Original Message----- >> From: Marek Vasut [mailto:marex@denx.de] >> Sent: Thursday, June 02, 2016 1:38 AM >> To: Rajesh Bhagat <rajesh.bhagat@nxp.com>; u-boot@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@denx.de] >>>> Sent: Wednesday, June 01, 2016 6:51 PM >>>> To: Rajesh Bhagat <rajesh.bhagat@nxp.com>; u-boot@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().
> -----Original Message----- > From: Marek Vasut [mailto:marex@denx.de] > Sent: Thursday, June 02, 2016 5:49 PM > To: Rajesh Bhagat <rajesh.bhagat@nxp.com>; u-boot@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@denx.de] > >> Sent: Thursday, June 02, 2016 1:38 AM > >> To: Rajesh Bhagat <rajesh.bhagat@nxp.com>; u-boot@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@denx.de] > >>>> Sent: Wednesday, June 01, 2016 6:51 PM > >>>> To: Rajesh Bhagat <rajesh.bhagat@nxp.com>; u-boot@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
On 06/17/2016 06:24 AM, Rajesh Bhagat wrote: > > >> -----Original Message----- >> From: Marek Vasut [mailto:marex@denx.de] >> Sent: Thursday, June 02, 2016 5:49 PM >> To: Rajesh Bhagat <rajesh.bhagat@nxp.com>; u-boot@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@denx.de] >>>> Sent: Thursday, June 02, 2016 1:38 AM >>>> To: Rajesh Bhagat <rajesh.bhagat@nxp.com>; u-boot@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@denx.de] >>>>>> Sent: Wednesday, June 01, 2016 6:51 PM >>>>>> To: Rajesh Bhagat <rajesh.bhagat@nxp.com>; u-boot@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.
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:
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(-)