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

[Kevin Wolf <kwolf@redhat.com>]

Am 21.05.2012 19:56, schrieb Blue Swirl:
> On Mon, May 21, 2012 at 10:25 AM, Jim Meyering <jim@meyering.net> wrote:
>> Blue Swirl wrote:
>>> On Tue, May 15, 2012 at 1:04 PM,  <jim@meyering.net> wrote:
>>>> From: Jim Meyering <meyering@redhat.com>
>>>>
>>>> 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 <meyering@redhat.com>
>>>> ---
>>>>  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.

A patch replacing tabs by spaces isn't really the kind of patches that
we would want to avoid during freeze. It's easy enough to check with git
diff -w that it doesn't change anything semantically.

Kevin