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

[Marek Vasut]

On 03/10/2017 06:51 PM, Nicolas le bayon wrote:
> 2017-03-10 17:52 GMT+01:00 Marek Vasut <marex at denx.de>:
> 
>> On 03/10/2017 05:31 PM, Nicolas le bayon wrote:
>>> From: Nicolas Le Bayon <nlebayon at 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 at 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
> 


-- 
Best regards,
Marek Vasut