This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH][BZ #14806] Fix stack overflow in getaddrinfo() if host has many addresses.
- From: Andreas Jaeger <aj at suse dot com>
- To: libc-alpha at sourceware dot org, mkubecek at suse dot cz
- Date: Mon, 10 Jun 2013 17:02:19 +0200
- Subject: Re: [PATCH][BZ #14806] Fix stack overflow in getaddrinfo() if host has many addresses.
- References: <20130530135411 dot B8F9062C80 at unicorn dot suse dot cz>
Hi Michal,
thanks for your patch. There're a couple of issues with the formatting
of the code, let me comment on them.
The full list of code conventions is at:
http://sourceware.org/glibc/wiki/Style_and_Conventions
On 05/30/2013 03:54 PM, Michal Kubecek wrote:
2013-05-29 Michal Kubecek <mkubecek@suse.cz>
[BZ #14806]
* sysdeps/unix/sysv/linux/check_pf.c: Do not allocate list of local
addresses on stack.
diff --git a/sysdeps/unix/sysv/linux/check_pf.c b/sysdeps/unix/sysv/linux/check_pf.c
index 34c2146..82fe9be 100644
--- a/sysdeps/unix/sysv/linux/check_pf.c
+++ b/sysdeps/unix/sysv/linux/check_pf.c
@@ -45,6 +45,9 @@
# define IFA_F_OPTIMISTIC 0
#endif
+#define IN6AI_FIXED_SIZE 16 /* size of fixed block allocated on stack */
+#define IN6AI_CHUNK_SIZE 64 /* size of linked list chunk */
+
Comments are always full sentences - starting with an uppercase letter,
closing with a "." and two spaces after, thus:
/* Size of linked list chunk. */
struct cached_data
{
@@ -62,6 +65,24 @@ static struct cached_data noai6ai_cached =
.in6ailen = 0
};
+/* The list of IPv6 addresses is usually very short but on some systems, it
+ can be very long (thousands of entries). So we want the implementation to
+ be fast for short lists but able to handle arbitrary length. Therefore we
+ keep one block of IN6AI_FIXED_SIZE entries on the stack and a linked list
+ of blocks of IN6AI_CHUNK_SIZE entries allocated by malloc() */
Two spaces after each "." and one "." at the end
+struct in6ai_chunk
+{
+ struct in6addrinfo info[IN6AI_CHUNK_SIZE];
+ struct in6ai_chunk *next;
+};
+
+struct in6ai_list
+{
+ struct in6addrinfo info[IN6AI_FIXED_SIZE];
+ unsigned count;
Always add the "int"
+ struct in6ai_chunk *next;
+};
+
libc_freeres_ptr (static struct cached_data *cache);
__libc_lock_define_initialized (static, lock);
@@ -103,6 +124,94 @@ cache_valid_p (void)
}
+static void
+in6ailist_init (struct in6ai_list *list)
+{
+ list->count = 0;
+ list->next = NULL;
+}
+
+
+/* Append a new entry to the list and return pointer to it */
Fix the comment, please (and those below as well)
+static struct in6addrinfo *
+in6ailist_add (struct in6ai_list *list)
+{
+ if (list->count < IN6AI_FIXED_SIZE)
+ return &list->info[list->count++];
+
+ unsigned idx = list->count - IN6AI_FIXED_SIZE;
unsigned int - please check the whole code.
+ struct in6ai_chunk *chunk = list->next;
+ while (idx > IN6AI_CHUNK_SIZE)
+ {
+ chunk = chunk->next;
+ idx -= IN6AI_CHUNK_SIZE;
+ }
+
+ if (idx == IN6AI_CHUNK_SIZE)
+ {
+ chunk->next = malloc (sizeof (struct in6ai_chunk));
+ if (!chunk->next)
+ return NULL;
+ chunk = chunk->next;
+ chunk->next = NULL;
+ idx = 0;
+ }
+ list->count++;
+ return &chunk->info[idx];
+}
+
+
+/* Export list as an array returned by make_request() */
+static struct cached_data *
+in6ailist_export (struct in6ai_list *list)
+{
+ struct cached_data *result;
+ unsigned count = list->count;
+ result = malloc (sizeof (struct cached_data)
+ + count * sizeof (struct in6addrinfo));
+ if (!result)
+ return NULL;
+
+ struct in6addrinfo *p = result->in6ai;
+ unsigned n;
+
+ n = (count > IN6AI_FIXED_SIZE) ? IN6AI_FIXED_SIZE : count;
+ memcpy (p, list->info, n * sizeof (struct in6addrinfo));
+ p += n;
+ count -= n;
+
+ struct in6ai_chunk *chunk = list->next;
+ while (n > 0)
+ {
+ n = (count > IN6AI_CHUNK_SIZE) ? IN6AI_CHUNK_SIZE : count;
+ memcpy (p, list->info, n * sizeof (struct in6addrinfo));
+ p += n;
+ count -= n;
+ chunk = chunk->next;
+ }
+
+ result->in6ailen = list->count;
+ return result;
+}
+
+
+/* Empty the list and free allocated chunks */
+static void
+in6ailist_cleanup (struct in6ai_list *list)
+{
+ struct in6ai_chunk *p = list->next;
+ struct in6ai_chunk *next;
+
+ while (p)
+ {
+ next = p->next;
+ free (p);
+ p = next;
+ }
+ list->count = 0;
+}
+
+
static struct cached_data *
make_request (int fd, pid_t pid)
{
@@ -130,6 +239,9 @@ make_request (int fd, pid_t pid)
memset (&nladdr, '\0', sizeof (nladdr));
nladdr.nl_family = AF_NETLINK;
+ struct in6ai_list in6ai_list;
+ in6ailist_init (&in6ai_list);
+
#ifdef PAGE_SIZE
/* Help the compiler optimize out the malloc call if PAGE_SIZE
is constant and smaller or equal to PTHREAD_STACK_MIN/4. */
@@ -159,12 +271,6 @@ make_request (int fd, pid_t pid)
goto out_fail;
bool done = false;
- struct in6ailist
- {
- struct in6addrinfo info;
- struct in6ailist *next;
- } *in6ailist = NULL;
- size_t in6ailistlen = 0;
bool seen_ipv4 = false;
bool seen_ipv6 = false;
@@ -239,28 +345,27 @@ make_request (int fd, pid_t pid)
}
}
- struct in6ailist *newp = alloca (sizeof (*newp));
- newp->info.flags = (((ifam->ifa_flags
- & (IFA_F_DEPRECATED
- | IFA_F_OPTIMISTIC))
- ? in6ai_deprecated : 0)
- | ((ifam->ifa_flags
- & IFA_F_HOMEADDRESS)
- ? in6ai_homeaddress : 0));
- newp->info.prefixlen = ifam->ifa_prefixlen;
- newp->info.index = ifam->ifa_index;
+ struct in6addrinfo *info = in6ailist_add (&in6ai_list);
+ if (!info)
+ goto out_fail;
+ info->flags = (((ifam->ifa_flags
+ & (IFA_F_DEPRECATED
+ | IFA_F_OPTIMISTIC))
+ ? in6ai_deprecated : 0)
+ | ((ifam->ifa_flags
+ & IFA_F_HOMEADDRESS)
+ ? in6ai_homeaddress : 0));
+ info->prefixlen = ifam->ifa_prefixlen;
+ info->index = ifam->ifa_index;
if (ifam->ifa_family == AF_INET)
{
- newp->info.addr[0] = 0;
- newp->info.addr[1] = 0;
- newp->info.addr[2] = htonl (0xffff);
- newp->info.addr[3] = *(const in_addr_t *) address;
+ info->addr[0] = 0;
+ info->addr[1] = 0;
+ info->addr[2] = htonl (0xffff);
+ info->addr[3] = *(const in_addr_t *) address;
}
else
- memcpy (newp->info.addr, address, sizeof (newp->info.addr));
- newp->next = in6ailist;
- in6ailist = newp;
- ++in6ailistlen;
+ memcpy (info->addr, address, sizeof (info->addr));
}
else if (nlmh->nlmsg_type == NLMSG_DONE)
/* We found the end, leave the loop. */
@@ -270,10 +375,9 @@ make_request (int fd, pid_t pid)
while (! done);
struct cached_data *result;
- if (seen_ipv6 && in6ailist != NULL)
+ if (seen_ipv6 && in6ai_list.count > 0)
{
- result = malloc (sizeof (*result)
- + in6ailistlen * sizeof (struct in6addrinfo));
+ result = in6ailist_export (&in6ai_list);
if (result == NULL)
goto out_fail;
@@ -281,14 +385,6 @@ make_request (int fd, pid_t pid)
result->usecnt = 2;
result->seen_ipv4 = seen_ipv4;
result->seen_ipv6 = true;
- result->in6ailen = in6ailistlen;
-
- do
- {
- result->in6ai[--in6ailistlen] = in6ailist->info;
- in6ailist = in6ailist->next;
- }
- while (in6ailist != NULL);
}
else
{
@@ -298,11 +394,13 @@ make_request (int fd, pid_t pid)
result = &noai6ai_cached;
}
+ in6ailist_cleanup (&in6ai_list);
if (use_malloc)
free (buf);
return result;
out_fail:
+ in6ailist_cleanup (&in6ai_list);
if (use_malloc)
free (buf);
return NULL;
Michal, do you have a proper copyright assignment with the FSF?
Andreas
--
Andreas Jaeger aj@{suse.com,opensuse.org} Twitter/Identica: jaegerandi
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn,Jennifer Guild,Felix Imendörffer,HRB16746 (AG Nürnberg)
GPG fingerprint = 93A3 365E CE47 B889 DF7F FED1 389A 563C C272 A126