drivers: usb: fsl: Fix NULL terminating issue for usb controller name string

PATCH New Rajesh Bhagat 2 participants 8 messages Jun 1, 2016 – Jun 17, 2016

Subsystems (2)

USB USB ehci driver

lore

Share & tools

lore

Rajesh Bhagat

author

2016-06-01 11:17 UTC #0 lore raw

PATCH 1/1 drivers: usb: fsl: Fix NULL terminating issue for usb controller name string

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 <[email protected]>

---
 drivers/usb/host/ehci-fsl.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

drivers/usb/host/ehci-fsl.c +4 -6

index a43d37d..a806993 100644
@@ -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:

Signature · 1 line

1.7.7.4

Marek Vasut

2016-06-01 13:20 UTC #1 lore raw

PATCH 1/1 drivers: usb: fsl: Fix NULL terminating issue for usb controller name string

Quoted 28 lines · 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 <[email protected]>

---
 drivers/usb/host/ehci-fsl.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

drivers/usb/host/ehci-fsl.c +2 -4

index a43d37d..a806993 100644
@@ -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()) {
 		/*

drivers/usb/host/ehci-fsl.c int ehci_hcd_init(int index, enum usb_init_type init, · line 66

Quoted 7 lines

drivers/usb/host/ehci-fsl.c +2 -2

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

Quoted 3 lines

drivers/usb/host/ehci-fsl.c +0 -0
 	switch (index) {
 	case 0:
 

Signature · 2 lines

Best regards,
Marek Vasut

Rajesh Bhagat

author

2016-06-01 14:55 UTC #2 lore raw

PATCH 1/1 drivers: usb: fsl: Fix NULL terminating issue for usb controller name string

Quoted 42 lines

-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Wednesday, June 01, 2016 6:51 PM
To: Rajesh Bhagat <[email protected]>; u-boot at lists.denx.de
Cc: york sun <[email protected]>; Sriram Dash <[email protected]>
Subject: Re: [PATCH] drivers: usb: fsl: Fix NULL terminating issue for usb controller name string
Quoted 31 lines · 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 <[email protected]>
---
 drivers/usb/host/ehci-fsl.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)
drivers/usb/host/ehci-fsl.c +2 -4
index a43d37d..a806993 100644
@@ -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);
 	}

Quoted 9 lines

drivers/usb/host/ehci-fsl.c int ehci_hcd_init(int index, enum usb_init_type init, · line 66
Quoted 4 lines
drivers/usb/host/ehci-fsl.c +2 -2
-	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

Quoted 8 lines

drivers/usb/host/ehci-fsl.c int ehci_hcd_init(int index, enum usb_init_type init,
Quoted 3 lines
drivers/usb/host/ehci-fsl.c +0 -0
 	switch (index) {
 	case 0:
 
--
Best regards,
Marek Vasut

Marek Vasut

2016-06-01 20:08 UTC #3 lore raw

PATCH 1/1 drivers: usb: fsl: Fix NULL terminating issue for usb controller name string

Quoted 52 lines · Rajesh Bhagat

Quoted 42 lines
-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Wednesday, June 01, 2016 6:51 PM
To: Rajesh Bhagat <[email protected]>; u-boot at lists.denx.de
Cc: york sun <[email protected]>; Sriram Dash <[email protected]>
Subject: Re: [PATCH] drivers: usb: fsl: Fix NULL terminating issue for usb controller name string
Quoted 31 lines · 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 <[email protected]>
---
 drivers/usb/host/ehci-fsl.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)
drivers/usb/host/ehci-fsl.c +2 -4
index a43d37d..a806993 100644
@@ -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);
 	}
Quoted 9 lines
drivers/usb/host/ehci-fsl.c int ehci_hcd_init(int index, enum usb_init_type init, · line 66
Quoted 4 lines
drivers/usb/host/ehci-fsl.c +2 -2
-	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.

Quoted 4 lines

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.

Quoted 30 lines

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
Quoted 8 lines
drivers/usb/host/ehci-fsl.c int ehci_hcd_init(int index, enum usb_init_type init,
Quoted 3 lines
drivers/usb/host/ehci-fsl.c +0 -0
 	switch (index) {
 	case 0:
 
--
Best regards,
Marek Vasut

Signature · 2 lines

Best regards,
Marek Vasut

Rajesh Bhagat

author

2016-06-02 03:14 UTC #4 lore raw

PATCH 1/1 drivers: usb: fsl: Fix NULL terminating issue for usb controller name string

Quoted 68 lines

-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Thursday, June 02, 2016 1:38 AM
To: Rajesh Bhagat <[email protected]>; u-boot at lists.denx.de
Cc: york sun <[email protected]>; Sriram Dash <[email protected]>
Subject: Re: [PATCH] drivers: usb: fsl: Fix NULL terminating issue for usb controller name string
Quoted 52 lines · Rajesh Bhagat
Quoted 42 lines
-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Wednesday, June 01, 2016 6:51 PM
To: Rajesh Bhagat <[email protected]>; u-boot at lists.denx.de
Cc: york sun <[email protected]>; Sriram Dash <[email protected]>
Subject: Re: [PATCH] drivers: usb: fsl: Fix NULL terminating issue for usb controller name string
Quoted 31 lines · 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 <[email protected]>
---
 drivers/usb/host/ehci-fsl.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)
(quoted diff) +0 -0
b/drivers/usb/host/ehci-fsl.c index a43d37d..a806993 100644
drivers/usb/host/ehci-fsl.c +2 -4
@@ -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);
 	}
Quoted 9 lines
drivers/usb/host/ehci-fsl.c int ehci_hcd_init(int index, enum usb_init_type init, · line 66
Quoted 4 lines
drivers/usb/host/ehci-fsl.c +2 -2
-	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.
Quoted 4 lines
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?

Quoted 6 lines

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

Quoted 35 lines

Quoted 13 lines
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,
Quoted 17 lines
sizeof(current_usb_controller), "usb%d", index+1); #endif
        printf("current_usb_controller %s\n", current_usb_controller);
        return 0;
}
Best Regards,
Rajesh Bhagat
Quoted 8 lines
drivers/usb/host/ehci-fsl.c int ehci_hcd_init(int index, enum usb_init_type init,
Quoted 3 lines
drivers/usb/host/ehci-fsl.c +0 -0
 	switch (index) {
 	case 0:
 
--
Best regards,
Marek Vasut
--
Best regards,
Marek Vasut

Marek Vasut

2016-06-02 12:18 UTC #5 lore raw

PATCH 1/1 drivers: usb: fsl: Fix NULL terminating issue for usb controller name string

Quoted 82 lines · Rajesh Bhagat

Quoted 68 lines
-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Thursday, June 02, 2016 1:38 AM
To: Rajesh Bhagat <[email protected]>; u-boot at lists.denx.de
Cc: york sun <[email protected]>; Sriram Dash <[email protected]>
Subject: Re: [PATCH] drivers: usb: fsl: Fix NULL terminating issue for usb controller name string
Quoted 52 lines · Rajesh Bhagat
Quoted 42 lines
-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Wednesday, June 01, 2016 6:51 PM
To: Rajesh Bhagat <[email protected]>; u-boot at lists.denx.de
Cc: york sun <[email protected]>; Sriram Dash <[email protected]>
Subject: Re: [PATCH] drivers: usb: fsl: Fix NULL terminating issue for usb controller name string
Quoted 31 lines · 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 <[email protected]>
---
 drivers/usb/host/ehci-fsl.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)
(quoted diff) +0 -0
b/drivers/usb/host/ehci-fsl.c index a43d37d..a806993 100644
drivers/usb/host/ehci-fsl.c +2 -4
@@ -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);
 	}
Quoted 9 lines
drivers/usb/host/ehci-fsl.c int ehci_hcd_init(int index, enum usb_init_type init, · line 66
Quoted 4 lines
drivers/usb/host/ehci-fsl.c +2 -2
-	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.
Quoted 4 lines
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?
Quoted 6 lines
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:

Quoted 6 lines

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

Signature · 2 lines

Best regards,
Marek Vasut

Rajesh Bhagat

author

2016-06-17 04:24 UTC #6 lore raw

PATCH 1/1 drivers: usb: fsl: Fix NULL terminating issue for usb controller name string

Quoted 100 lines

-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Thursday, June 02, 2016 5:49 PM
To: Rajesh Bhagat <[email protected]>; u-boot at lists.denx.de
Cc: york sun <[email protected]>; Sriram Dash <[email protected]>
Subject: Re: [PATCH] drivers: usb: fsl: Fix NULL terminating issue for usb controller name string
Quoted 53 lines · Rajesh Bhagat
Quoted 53 lines
-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Thursday, June 02, 2016 1:38 AM
To: Rajesh Bhagat <[email protected]>; u-boot at lists.denx.de
Cc: york sun <[email protected]>; Sriram Dash <[email protected]>
Subject: Re: [PATCH] drivers: usb: fsl: Fix NULL terminating issue for usb controller name string
Quoted 42 lines · Rajesh Bhagat
Quoted 42 lines
-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Wednesday, June 01, 2016 6:51 PM
To: Rajesh Bhagat <[email protected]>; u-boot at lists.denx.de
Cc: york sun <[email protected]>; Sriram Dash <[email protected]>
Subject: Re: [PATCH] drivers: usb: fsl: Fix NULL terminating issue for usb controller name string
Quoted 31 lines · 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 <[email protected]>
---
 drivers/usb/host/ehci-fsl.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)
(quoted diff) +0 -0
b/drivers/usb/host/ehci-fsl.c index a43d37d..a806993 100644
drivers/usb/host/ehci-fsl.c +2 -4
@@ -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);
 	}
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/usb/host/ehci-fsl.c int ehci_hcd_init(int index, enum usb_init_type init, · line 66
Quoted 4 lines
drivers/usb/host/ehci-fsl.c +2 -2
-	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.
Quoted 9 lines
Quoted 9 lines
Quoted 6 lines
Quoted 1 line
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:
Quoted 8 lines
Quoted 4 lines
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:
Quoted 9 lines
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

Quoted 4 lines

Quoted 3 lines
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,

Quoted 3 lines

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

Are you planning to pick this up?

Best Regards,
Rajesh Bhagat

Quoted 3 lines

--
Best regards,
Marek Vasut

Marek Vasut

2016-06-17 19:51 UTC #7 lore raw

PATCH 1/1 drivers: usb: fsl: Fix NULL terminating issue for usb controller name string

Quoted 105 lines · Rajesh Bhagat

Quoted 100 lines
-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Thursday, June 02, 2016 5:49 PM
To: Rajesh Bhagat <[email protected]>; u-boot at lists.denx.de
Cc: york sun <[email protected]>; Sriram Dash <[email protected]>
Subject: Re: [PATCH] drivers: usb: fsl: Fix NULL terminating issue for usb controller name string
Quoted 53 lines · Rajesh Bhagat
Quoted 53 lines
-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Thursday, June 02, 2016 1:38 AM
To: Rajesh Bhagat <[email protected]>; u-boot at lists.denx.de
Cc: york sun <[email protected]>; Sriram Dash <[email protected]>
Subject: Re: [PATCH] drivers: usb: fsl: Fix NULL terminating issue for usb controller name string
Quoted 42 lines · Rajesh Bhagat
Quoted 42 lines
-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Wednesday, June 01, 2016 6:51 PM
To: Rajesh Bhagat <[email protected]>; u-boot at lists.denx.de
Cc: york sun <[email protected]>; Sriram Dash <[email protected]>
Subject: Re: [PATCH] drivers: usb: fsl: Fix NULL terminating issue for usb controller name string
Quoted 31 lines · 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 <[email protected]>
---
 drivers/usb/host/ehci-fsl.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)
(quoted diff) +0 -0
b/drivers/usb/host/ehci-fsl.c index a43d37d..a806993 100644
drivers/usb/host/ehci-fsl.c +2 -4
@@ -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);
 	}
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/usb/host/ehci-fsl.c int ehci_hcd_init(int index, enum usb_init_type init, · line 66
Quoted 4 lines
drivers/usb/host/ehci-fsl.c +2 -2
-	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.
Quoted 9 lines
Quoted 9 lines
Quoted 6 lines
Quoted 1 line
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:
Quoted 8 lines
Quoted 4 lines
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:
Quoted 9 lines
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
Quoted 4 lines
Quoted 3 lines
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.

Quoted 7 lines

Hello Marek,
Quoted 3 lines
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.

Signature · 2 lines

Best regards,
Marek Vasut