[U-Boot] [PATCH v2] usb: gadget: dynamic envstr size in cb_getvar
Hi,
here is a second patch proposal with a dynamic size allocation for evstr in cb_getvar.
Thanks in advance for your feedback/approval. Best Regards Nicolas
---
--- drivers/usb/gadget/f_fastboot.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 2160b1c..8b73277 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -432,9 +432,11 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req) else strcpy(response, "FAILValue not set"); } else { - char envstr[32]; + char *envstr;
- snprintf(envstr, sizeof(envstr) - 1, "fastboot.%s", cmd); + envstr = malloc(sizeof("fastboot.%s", cmd) + 1); + + sprintf(envstr, "fastboot.%s", cmd); s = getenv(envstr); if (s) { strncat(response, s, chars_left);
On 03/10/2017 12:03 PM, Nicolas le bayon wrote:
Hi,
here is a second patch proposal with a dynamic size allocation for evstr in cb_getvar.
Thanks in advance for your feedback/approval.
Please write a sensible commit message ...
Use git send-email to send a patch.
Best Regards Nicolas
drivers/usb/gadget/f_fastboot.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 2160b1c..8b73277 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -432,9 +432,11 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req) else strcpy(response, "FAILValue not set"); } else {
- char envstr[32];
- char *envstr;
- snprintf(envstr, sizeof(envstr) - 1, "fastboot.%s", cmd);
- envstr = malloc(sizeof("fastboot.%s", cmd) + 1);
You never even compile-tested this, did you ? There is so much wrong with this patch I wanna cry ...
sizeof() takes one argument, if you bothered compiling the patch, compiler would tell you.
If you fix the sizeof, it will return 12, always (because 11 chars + \0 at the end of string), so that cannot work. Use strlen to figure out the size of the fastboot prefix and the size of cmd. Then malloc the correct size.
malloc() can fail and return NULL, YES THIS REALLY HAPPENS (!!!). ALWAYS check the return value of malloc, ALWAYS.
- sprintf(envstr, "fastboot.%s", cmd); s = getenv(envstr); if (s) { strncat(response, s, chars_left);
You're leaking memory because you never free() the allocated mem.
Oh sorry Marek to make you lose your time, I've just seen that I sent you an intermediate release of my patch, it wasn't the expected one. This is totally my fault, sorry for that.
Anyway I'll also take into account some of your remarks which wasn't treated.
I'll propose you a v3 release as soon as possible.
Sorry agan Best Regards Nicolas
2017-03-10 13:22 GMT+01:00 Marek Vasut marex@denx.de:
On 03/10/2017 12:03 PM, Nicolas le bayon wrote:
Hi,
here is a second patch proposal with a dynamic size allocation for evstr
in
cb_getvar.
Thanks in advance for your feedback/approval.
Please write a sensible commit message ...
Use git send-email to send a patch.
Best Regards Nicolas
drivers/usb/gadget/f_fastboot.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 2160b1c..8b73277 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -432,9 +432,11 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req) else strcpy(response, "FAILValue not set"); } else {
- char envstr[32];
- char *envstr;
- snprintf(envstr, sizeof(envstr) - 1, "fastboot.%s", cmd);
- envstr = malloc(sizeof("fastboot.%s", cmd) + 1);
You never even compile-tested this, did you ? There is so much wrong with this patch I wanna cry ...
sizeof() takes one argument, if you bothered compiling the patch, compiler would tell you.
If you fix the sizeof, it will return 12, always (because 11 chars + \0 at the end of string), so that cannot work. Use strlen to figure out the size of the fastboot prefix and the size of cmd. Then malloc the correct size.
malloc() can fail and return NULL, YES THIS REALLY HAPPENS (!!!). ALWAYS check the return value of malloc, ALWAYS.
- sprintf(envstr, "fastboot.%s", cmd); s = getenv(envstr); if (s) { strncat(response, s, chars_left);
You're leaking memory because you never free() the allocated mem.
-- Best regards, Marek Vasut
On 03/10/2017 03:28 PM, Nicolas le bayon wrote:
Oh sorry Marek to make you lose your time, I've just seen that I sent you an intermediate release of my patch, it wasn't the expected one. This is totally my fault, sorry for that.
Anyway I'll also take into account some of your remarks which wasn't treated.
I'll propose you a v3 release as soon as possible.
Look at git send-email --annotate , helps avoiding such flubs ...
Sorry agan Best Regards Nicolas
2017-03-10 13:22 GMT+01:00 Marek Vasut marex@denx.de:
On 03/10/2017 12:03 PM, Nicolas le bayon wrote:
Hi,
here is a second patch proposal with a dynamic size allocation for evstr
in
cb_getvar.
Thanks in advance for your feedback/approval.
Please write a sensible commit message ...
Use git send-email to send a patch.
Best Regards Nicolas
drivers/usb/gadget/f_fastboot.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 2160b1c..8b73277 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -432,9 +432,11 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req) else strcpy(response, "FAILValue not set"); } else {
- char envstr[32];
- char *envstr;
- snprintf(envstr, sizeof(envstr) - 1, "fastboot.%s", cmd);
- envstr = malloc(sizeof("fastboot.%s", cmd) + 1);
You never even compile-tested this, did you ? There is so much wrong with this patch I wanna cry ...
sizeof() takes one argument, if you bothered compiling the patch, compiler would tell you.
If you fix the sizeof, it will return 12, always (because 11 chars + \0 at the end of string), so that cannot work. Use strlen to figure out the size of the fastboot prefix and the size of cmd. Then malloc the correct size.
malloc() can fail and return NULL, YES THIS REALLY HAPPENS (!!!). ALWAYS check the return value of malloc, ALWAYS.
- sprintf(envstr, "fastboot.%s", cmd); s = getenv(envstr); if (s) { strncat(response, s, chars_left);
You're leaking memory because you never free() the allocated mem.
-- Best regards, Marek Vasut
participants (2)
-
Marek Vasut -
Nicolas le bayon