On Mon, May 21, 2012 at 10:25 AM, Jim Meyering <j...@meyering.net> wrote:
> Blue Swirl wrote:
>> On Tue, May 15, 2012 at 1:04 PM, <j...@meyering.net> wrote:
>>> From: Jim Meyering <meyer...@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 <meyer...@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.
>
> ...
>> total: 9 errors, 0 warnings, 15 lines checked
>>
>> Please fix.
