[U-Boot] [PATCH v3] usb: gadget: dynamic envstr size in cb_getvar
From: Nicolas Le Bayon nlebayon@gmail.com
use dynamic allocation to consider all variable lengths
Signed-off-by: Nicolas Le Bayon nlebayon@gmail.com --- drivers/usb/gadget/f_fastboot.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 2160b1c..9bb3a95 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -432,9 +432,19 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req) else strcpy(response, "FAILValue not set"); } else { - char envstr[32]; + char *envstr = NULL; + unsigned int len;
- snprintf(envstr, sizeof(envstr) - 1, "fastboot.%s", cmd); + len = strlen("fastboot.") + strlen(cmd) + 1; + + envstr = malloc(len); + if (!envstr) { + error("variable malloc error"); + fastboot_tx_write_str("FAILvar malloc error"); + return; + } + + sprintf(envstr, "fastboot.%s", cmd); s = getenv(envstr); if (s) { strncat(response, s, chars_left); @@ -442,6 +452,8 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req) printf("WARNING: unknown variable: %s\n", cmd); strcpy(response, "FAILVariable not implemented"); } + + free(envstr); } fastboot_tx_write_str(response); }
On 03/10/2017 05:31 PM, Nicolas le bayon wrote:
From: Nicolas Le Bayon nlebayon@gmail.com
use dynamic allocation to consider all variable lengths
Of what ? Where ? Why ?
Signed-off-by: Nicolas Le Bayon nlebayon@gmail.com
drivers/usb/gadget/f_fastboot.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 2160b1c..9bb3a95 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -432,9 +432,19 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req) else strcpy(response, "FAILValue not set"); } else {
- char envstr[32];
- char *envstr = NULL;
This var is inited by malloc() below, drop the = NULL .
- unsigned int len;
- snprintf(envstr, sizeof(envstr) - 1, "fastboot.%s", cmd);
- len = strlen("fastboot.") + strlen(cmd) + 1;
- envstr = malloc(len);
The len is only used here, just do malloc(strlen ..... ));
- if (!envstr) {
- error("variable malloc error");
This error message makes no sense ...
- fastboot_tx_write_str("FAILvar malloc error");
The indent of this whole block is wrong ...
- return;
- }
- sprintf(envstr, "fastboot.%s", cmd); s = getenv(envstr); if (s) { strncat(response, s, chars_left);
@@ -442,6 +452,8 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req) printf("WARNING: unknown variable: %s\n", cmd); strcpy(response, "FAILVariable not implemented"); }
- free(envstr); } fastboot_tx_write_str(response);
}
2017-03-10 17:52 GMT+01:00 Marek Vasut marex@denx.de:
On 03/10/2017 05:31 PM, Nicolas le bayon wrote:
From: Nicolas Le Bayon nlebayon@gmail.com
use dynamic allocation to consider all variable lengths
Of what ? Where ? Why ?
Here is a proposal of updated label: usb: gadget: dynamic alloc for variable name in cb_getvar
And a better description proposal: Allocate dynamically the buffer of the variable name in cb_getvar function This allows to treat correctly all variable name lengths, growing through releases, and also avoids cutting their names, leading to undefined variable for the function.
Signed-off-by: Nicolas Le Bayon nlebayon@gmail.com
drivers/usb/gadget/f_fastboot.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 2160b1c..9bb3a95 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -432,9 +432,19 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req) else strcpy(response, "FAILValue not set"); } else {
- char envstr[32];
- char *envstr = NULL;
This var is inited by malloc() below, drop the = NULL .
OK
- unsigned int len;
- snprintf(envstr, sizeof(envstr) - 1, "fastboot.%s", cmd);
- len = strlen("fastboot.") + strlen(cmd) + 1;
- envstr = malloc(len);
The len is only used here, just do malloc(strlen ..... ));
OK I merge all this in a single line
- if (!envstr) {
- error("variable malloc error");
This error message makes no sense ...
Is error("malloc error"); enough from your point of view?
- fastboot_tx_write_str("FAILvar malloc error");
The indent of this whole block is wrong ...
Regarding previous remark on error message, I also modified it to fastboot_tx_write_str("FAILmalloc error");
Regarding indent, it seems to me that it fits with the rest of the function, but I'm surely wrong, could you precise me the problem please?
- return;
- }
- sprintf(envstr, "fastboot.%s", cmd); s = getenv(envstr); if (s) { strncat(response, s, chars_left);
@@ -442,6 +452,8 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req) printf("WARNING: unknown variable: %s\n", cmd); strcpy(response, "FAILVariable not implemented"); }
- free(envstr); } fastboot_tx_write_str(response);
}
-- Best regards, Marek Vasut
Regars Nicolas Le Bayon
On 03/10/2017 06:51 PM, Nicolas le bayon wrote:
2017-03-10 17:52 GMT+01:00 Marek Vasut marex@denx.de:
On 03/10/2017 05:31 PM, Nicolas le bayon wrote:
From: Nicolas Le Bayon nlebayon@gmail.com
use dynamic allocation to consider all variable lengths
Of what ? Where ? Why ?
Here is a proposal of updated label: usb: gadget: dynamic alloc for variable name in cb_getvar
But you're fixing the problem of long env variables being clipped, that's what this patch does. It's not about the dynamic allocation at all. The dynamic allocation is the way you solved it and it should be in the patch description .
And a better description proposal: Allocate dynamically the buffer of the variable name in cb_getvar function This allows to treat correctly all variable name lengths, growing through releases, and also avoids cutting their names, leading to undefined variable for the function.
Signed-off-by: Nicolas Le Bayon nlebayon@gmail.com
drivers/usb/gadget/f_fastboot.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 2160b1c..9bb3a95 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -432,9 +432,19 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req) else strcpy(response, "FAILValue not set"); } else {
- char envstr[32];
- char *envstr = NULL;
This var is inited by malloc() below, drop the = NULL .
OK
- unsigned int len;
- snprintf(envstr, sizeof(envstr) - 1, "fastboot.%s", cmd);
- len = strlen("fastboot.") + strlen(cmd) + 1;
- envstr = malloc(len);
The len is only used here, just do malloc(strlen ..... ));
OK I merge all this in a single line
- if (!envstr) {
- error("variable malloc error");
This error message makes no sense ...
Is error("malloc error"); enough from your point of view?
What do you think ? Is it enough and is it helpful error message ?
- fastboot_tx_write_str("FAILvar malloc error");
The indent of this whole block is wrong ...
Regarding previous remark on error message, I also modified it to fastboot_tx_write_str("FAILmalloc error");
Regarding indent, it seems to me that it fits with the rest of the function, but I'm surely wrong, could you precise me the problem please?
Then maybe look at the patch: https://patchwork.ozlabs.org/patch/737452/
it's completely flat, which definitely doesn't match the code. Did you use git send-email to send the patch ?
- return;
- }
- sprintf(envstr, "fastboot.%s", cmd); s = getenv(envstr); if (s) { strncat(response, s, chars_left);
@@ -442,6 +452,8 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req) printf("WARNING: unknown variable: %s\n", cmd); strcpy(response, "FAILVariable not implemented"); }
- free(envstr); } fastboot_tx_write_str(response);
}
-- Best regards, Marek Vasut
Regars Nicolas Le Bayon
participants (2)
-
Marek Vasut -
Nicolas le bayon