[U-Boot] [PATCH v2] usb: gadget: dynamic envstr size in cb_getvar

[Nicolas le bayon]

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 at 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
>