Re: [PATCH v2 1/3] Fix __check_pf()/make_request() stack overflow segfault (convert to malloc)

[Pengcheng Chen <pengcheng dot chen at gmail dot com>]

On Thu, Jan 16, 2014 at 4:22 PM, OndÅej BÃlka <neleai@seznam.cz> wrote:
> On Thu, Jan 16, 2014 at 05:58:31PM -0500, Banerjee, Debabrata wrote:
>> On 1/16/14 5:53 PM, "OndÅej BÃlka" <neleai@seznam.cz> wrote:
>>
>> >On Thu, Oct 31, 2013 at 09:10:11PM -0400, Debabrata Banerjee wrote:
>> >> Tested calls to getaddrinfo() with 64k+ local IPv4 and IPv6 addresses.
>> >>
>> >> Changelog:
>> >>
>> >> 2013-10-31  Debabrata Banerjee  <dbanerje@akamai.com>
>> >>
>> >
>> >As that patch was quite large and there is not much time if we want this
>> >in 2.19 we should use a simpler patch. I wrote new patch that uses
>> >malloc instead.
>>
>> Sorry I haven't had time to look into refactoring this patch more. However
>> Pengcheng has volunteered to clean it up in whatever way is acceptable.
>>
>> The caveat with using malloc is that I would expect it to be significantly
>> slower than alloca, but someone can benchmark that. I believe this is a
>> hot path.
>>
> That is tricky question, bottleneck could also be in network
> latency, probably here
>
>  if (TEMP_FAILURE_RETRY (__sendto (fd, (void *) &req, sizeof (req), 0,
>                                     (struct sockaddr *) &nladdr,
>                                     sizeof (nladdr))) < 0)
>
> Besides of that as I studied a code more carefully I probably found that
> these allocations are useless.
>
> We use these entries to store data in linked list, relevant part is
>
>        struct in6ailist *newp = alloca (sizeof (*newp));
>        <snip>
>        newp->next = in6ailist;
>        in6ailist = newp;
>        ++in6ailistlen;
>
> t we use this list only in following fragment:
>
>      result = malloc (sizeof (*result)
>                        + in6ailistlen * sizeof (struct in6addrinfo));
>       <snip>
>       do
>         {
>           result->in6ai[--in6ailistlen] = in6ailist->info;
>           in6ailist = in6ailist->next;
>         }
>       while (in6ailist != NULL);
>
> which only copies list into malloced array while preserving ordering.
>
> A better way would be to malloc result at start and write into in6ai
> array directly, calling realloc to double size as necessary. At end we
> could optionally trim memory at end.
>
> This should also be more effective as one big copy is faster when you
> need to copy same amount of data in small chunks.
Good suggestion. I totally agree with you. This make code simpler.
>
> Pengcheng could you do this?
Before I take on this task, can you please explain a little bit about
the process? I'm new to glibc and am still sort of struggling to build
it.

Thanks,
Pengcheng