| Message ID | CAJZhe_gOe44mafHdsEr7k84MxVWYiUzPCZo4mSwt66OJ2AkwWw@mail.gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Łukasz Majewski |
| Headers | show |
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 >
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) {