This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Fix __check_pf()/make_request() stack overflow segfault (convert to alloca_account) and memory leak.


On 10/4/13 12:33 AM, "Carlos O'Donell" <carlos@redhat.com> wrote:
>On 10/03/2013 05:51 PM, Debabrata Banerjee wrote:
>
>I'll review this right now so you can incorporate the review into your
>next
>version.

My mail client had actually truncated everything before this line. I
didn't see your review until I saw it on the archive a few days ago. Don't
ask :(

>
>All the code after this in the body of the if has the wrong indentation.
>This code also has the wrong indentation.
>
>All of this has to indent correctly against the starting brace.

I believe I fixed your indentation concern, although I couldn't guess at
what the indentation style actually is, the existing code is a mix of tabs
and spaces. I can clean up the indentation in the whole file in another
patch.

>
>Is there no way we can refactor this into a function?
>
>I would really like to avoid the duplication.
>
>The refactoring would be an extra patch, but would really
>clean this up.

I refactored my code to avoid the duplication without using a function.

>
>>  	  else if (nlmh->nlmsg_type == NLMSG_DONE)
>>  	    /* We found the end, leave the loop.  */
>>  	    done = true;
>> @@ -269,20 +327,15 @@ make_request (int fd, pid_t pid)
>>      }
>>    while (! done);
>>  
>> -  struct cached_data *result;
>> -  if (seen_ipv6 && in6ailist != NULL)
>> +  if (seen_ipv6)
>> +    {
>> +      if (result == NULL && in6ailist != NULL)
>>      {
>>        result = malloc (sizeof (*result)
>>  		       + in6ailistlen * sizeof (struct in6addrinfo));
>>        if (result == NULL)
>>  	goto out_fail;
>>  
>> -      result->timestamp = get_nl_timestamp ();
>> -      result->usecnt = 2;
>> -      result->seen_ipv4 = seen_ipv4;
>> -      result->seen_ipv6 = true;
>> -      result->in6ailen = in6ailistlen;
>> -
>>        do
>>  	{
>>  	  result->in6ai[--in6ailistlen] = in6ailist->info;
>> @@ -290,9 +343,29 @@ make_request (int fd, pid_t pid)
>>  	}
>>        while (in6ailist != NULL);
>>      }
>> +
>> +#ifdef IS_IN_nscd
>> +      result->timestamp = nl_timestamp;
>> +      result->usecnt = 2;
>> +#elif defined(USE_NSCD)
>> +      result->timestamp = __nscd_get_nl_timestamp ();
>> +      result->usecnt = 2;
>> +#else
>> +      result->usecnt = 1;
>> +#endif
>
>This looks like a fixup for an incomplete transition
>for supporting IS_IN_nscd/USE_NSDCD which should be
>a distinct patch.

I believe it was the NSCD patch that introduced the leak. However I split
it into another patch.

Please review the next patches.

Thanks,
Debabrata



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]