This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Fix __check_pf()/make_request() stack overflow segfault (convert to alloca_account) and memory leak.
- From: "Banerjee, Debabrata" <dbanerje at akamai dot com>
- To: Carlos O'Donell <carlos at redhat dot com>
- Cc: "libc-alpha at sourceware dot org" <libc-alpha at sourceware dot org>
- Date: Thu, 31 Oct 2013 21:09:06 -0400
- Subject: Re: [PATCH] Fix __check_pf()/make_request() stack overflow segfault (convert to alloca_account) and memory leak.
- Authentication-results: sourceware.org; auth=none
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