diff mbox

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

Message ID CAJZhe_iW1AcFt=1GWt2JzuRhfvWutDJtMuhX6rf++G8j2GDHqg@mail.gmail.com
State Superseded
Delegated to: Łukasz Majewski
Headers show

Commit Message

Nicolas le bayon March 10, 2017, 11:03 a.m. UTC 
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(-)

  strncat(response, s, chars_left);

Comments

Marek Vasut March 10, 2017, 12:22 p.m. UTC | #1 
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.
Nicolas le bayon March 10, 2017, 2:28 p.m. UTC | #2 
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
>
Marek Vasut March 10, 2017, 3:05 p.m. UTC | #3 
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
>>
>
diff mbox

Patch

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) {