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

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


-- 
Best regards,
Marek Vasut