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 theterminating 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().