qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 1/3] envlist.c: handle strdup failure


From: Blue Swirl
Subject: Re: [Qemu-devel] [PATCH 1/3] envlist.c: handle strdup failure
Date: Mon, 21 May 2012 17:56:32 +0000

On Mon, May 21, 2012 at 10:25 AM, Jim Meyering <address@hidden> wrote:
> Blue Swirl wrote:
>> On Tue, May 15, 2012 at 1:04 PM,  <address@hidden> wrote:
>>> From: Jim Meyering <address@hidden>
>>>
>>> Without this, envlist_to_environ may silently fail to copy all
>>> strings into the destination buffer, and both callers would leak
>>> any env strings allocated after a failing strdup, because the
>>> freeing code stops at the first NULL pointer.
>>>
>>> Signed-off-by: Jim Meyering <address@hidden>
>>> ---
>>>  envlist.c | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/envlist.c b/envlist.c
>>> index f2303cd..2bbd99c 100644
>>> --- a/envlist.c
>>> +++ b/envlist.c
>>> @@ -235,7 +235,14 @@ envlist_to_environ(const envlist_t *envlist, size_t 
>>> *count)
>>>
>>>        for (entry = envlist->el_entries.lh_first; entry != NULL;
>>>            entry = entry->ev_link.le_next) {
>>> -               *(penv++) = strdup(entry->ev_var);
>>> +               if ((*(penv++) = strdup(entry->ev_var)) == NULL) {
>>> +                       char **e = env;
>>> +                       while (e != penv) {
>>> +                               free(*e++);
>>> +                       }
>>> +                       free(env);
>>> +                       return NULL;
>>> +               }
>>
>> ERROR: code indent should never use tabs
>> #82: FILE: envlist.c:238:
>> +^I^Iif ((*(penv++) = strdup(entry->ev_var)) == NULL) {$
>
> That entire file is indented solely with TABs, so adding these new
> lines using spaces for indentation seems unjustified: the mix tends
> to make the code unreadable in some contexts (email quoting, for one).
> How about two patches: one to convert all leading TABs in envlist.c to
> spaces, and the next to make the above change, but indenting with spaces?
>
>> ERROR: do not use assignment in if condition
>> #82: FILE: envlist.c:238:
>> +               if ((*(penv++) = strdup(entry->ev_var)) == NULL) {
>
> I agree with the sentiment, but found that the alternative was less
> readable and less maintainable, since I'd have to increment "penv" in
> two places (both in the if-block and after it) rather than in just one.
> However, I've just realized I can hoist the "penv++" increment into the
> "for-statement", in which case it's ok:
>
>        for (entry = envlist->el_entries.lh_first; entry != NULL;
>             entry = entry->ev_link.le_next, penv++) {
>                *penv = strdup(entry->ev_var);
>                if (*penv == NULL) {
>                        char **e = env;
>                        while (e <= penv) {
>                                free(*e++);
>                        }
>                        free(env);
>                        return NULL;
>                }
>        }
>
> Your move.  Which would you prefer?
>  1) two patches: one replacing all leading TABs with equivalent spaces,
>      then the above patch
>  2) one patch, indented using TABs, in spite of the checkpatch failure
>  3) one patch, indented using spaces, in spite of the consistency issue

1) (or 3). Though for v1.1, maybe 3) is the smaller fix and later do 1) for 1.2.

>
> ...
>> total: 9 errors, 0 warnings, 15 lines checked
>>
>> Please fix.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]