small memory leak in libc/inet/resolv.c

[Bernhard Reutner-Fischer]

On Fri, Oct 14, 2011 at 09:12:35AM +0200, Joakim Tjernlund wrote:
>
>>
>> We've discovered a small memory leak in __dns_lookup() when the A record
>> in the DNS answer is preceded by one or more CNAME records.
>>
>> This can be reproduced by performing a gethostbyname() lookup on
>> "www.google.com" or "www.yahoo.com".  After each CNAME record,
>> __dns_lookup() leaks by 256 bytes.
>>
>> =====================================
>> The problem code is in this segment:
>>
>>         first_answer = 1;
>>         for (j = 0; j < h.ancount; j++) {
>>             i = __decode_answer(packet, pos, packet_len, &ma);
>>
>>             <...snipped code for brevity...>
>>
>>             if (first_answer) {
>>                 ma.buf = a->buf;
>>                 ma.buflen = a->buflen;
>>                 ma.add_count = a->add_count;
>>                 memcpy(a, &ma, sizeof(ma));
>>                 if (a->atype != T_SIG && (NULL == a->buf || (type != T_A && type != T_AAAA)))
>>                     break;
>>                 if (a->atype != type)
>>                     continue;
>>                 a->add_count = h.ancount - j - 1;
>>                 if ((a->rdlength + sizeof(struct in_addr*)) * a->add_count > a->buflen)
>>                     break;
>>                 a->add_count = 0;
>>                 first_answer = 0;
>>             } else {
>> =====================================
>>
>> On the first time through the loop (j == 0) with the CNAME record,
>> ma.dotted is alloc'ed using a strdup() in decode_answer(), and memcpy()
>> copies it to a->dotted. The loop is continued at "if (a->atype != type)".
>>
>> On the second time through the loop (j == 1), ma.dotted is again
>> alloc'ed in decode_answer().  Now we have two allocated segments, one
>> pointed-to by ma.dotted, and the other pointed-to by a->dotted.
>> But memcpy() will over-write the pointer stored at a->dotted.
>>
>> My proposed solution is to check a->dotted before the memcpy() and free()
>> it if it is non-NULL. (See the attached patch.)
>>
>> I've tested my solution with a number of different public DNS servers
>> resolving a variety of names, but we're using an older version of uClibc.
>>
>> This leak was introduced in April 2010 when a similar free() call was
>> removed for the test case when a DNS server returns a lone CNAME record.
>> (ipv6.google.com)
>
>free can handle NULL so you can skip the
>  if (a->dotted)
>test.

Applied, thanks!