Possible race condition in gwlib/protected.c

Hi,

I'm feeling my way into the kannel source, and I think I've identified
a race condition in the gw_gethostbyname() function in gwlib/protected.c
In particular, the gethostbyname function call returns some static data.

I've rolled patch to fix this, noting that:
1. The only callers in the of gw_gethostbyname in tree currently are in
   gwlib/socket.c
2. Those callers only use the hostent.h_name and hostent.h_addr_list[0]
   fields in the struct hostent.
3. I've been careful to free the memory allocated as close to the call
   to gw_gethostbyname() as possible.

The patch thus copies mallocs space for these fields, and copies the
relevant data, and then frees it.

I know that the BSDs don't support gethostbyname_r() function call (and
that it's a pig to make portable, and isn't part of the standards).  Is
there a reason Kannel uses gethostbyname in preference to getaddrinfo()?

Regards

Kieran



diff -ur -x '*.o' cvs/gateway/gwlib/protected.c work/gateway/gwlib/protected.c
--- cvs/gateway/gwlib/protected.c	2000-10-23 18:56:05.000000000 +0100
+++ work/gateway/gwlib/protected.c	2002-08-21 04:00:55.000000000 +0100
@@ -92,19 +92,69 @@
     return ret;
 }

+/*  This function:
+    1. Mallocs enough memory to copy relevent fields of src
+    2. Copies those fields into dest
+    This is done because the struct that gethostbyname returns contains pointers
+    to static data.
+    Note: since kannel only uses the hostent.h_name and hostent.h_addr_list[0]
+    elements of the returned hostent currently, that's all that is copied.
+*/
+void malloc_and_copy_hostent (struct hostent * dest, const struct hostent * src) {
+    int count;
+    char *cp;

+    /* Mallocs and copies hostent.h_name element */
+    count = strlen (src->h_name) + 1;
+    dest->h_name = (char *)gw_native_malloc (count);
+    memcpy (dest->h_name, src->h_name, count);
+
+    /* Mallocs an array of size 2 for hostent.h_addr_list */
+    count = sizeof (src->h_addr_list);
+    dest->h_addr_list = (char **)gw_native_malloc (2 * sizeof(char *));
+
+    /* Mallocs and copies first element of src->h_addr_list[] */
+    count = strlen (src->h_addr_list[0]) + 1;
+    cp = (char *)gw_native_malloc (count);
+    memcpy(cp, src->h_addr_list[0], count);
+
+    /* Sets h_addr_list[0] to copy, and [1] to NULL */
+    dest->h_addr_list[0] = cp;
+    dest->h_addr_list[1] = NULL;
+
+    return;
+}
+
+/* This function frees allocated hostent structures, as malloced by
+   malloc_and_copy_hostent() defined above.  This function needs to
+   be called every time gw_gethostbyname() is called to avoid memory
+   leaks
+*/
+void free_hostent(struct hostent * in) {
+    int i;
+    gw_free (in->h_name);
+    for (i=0; in->h_addr_list[i] != NULL; i++) {
+        gw_free (in->h_addr_list[i]);
+    }
+    gw_free (in->h_addr_list);
+}
+
+/* Pre:  The pointer "ent" must point to a valid struct hostent
+   Post: if the return value == 0, fields in ent point to malloced
+         memory which should be freed using free_hostent()
+*/
 int gw_gethostbyname(struct hostent *ent, const char *name)
 {
     int ret;
     struct hostent *p;

     lock(GETHOSTBYNAME);
-    p = gethostbyname(name);
+    p = gethostbyname(name);
     if (p == NULL)
         ret = -1;
     else {
         ret = 0;
-        *ent = *p;
+        malloc_and_copy_hostent(ent, p);
     }
     unlock(GETHOSTBYNAME);
     return ret;
diff -ur -x '*.o' cvs/gateway/gwlib/protected.h work/gateway/gwlib/protected.h
--- cvs/gateway/gwlib/protected.h	2001-01-29 13:01:43.000000000 +0000
+++ work/gateway/gwlib/protected.h	2002-08-21 03:36:35.000000000 +0100
@@ -21,6 +21,7 @@
 struct tm gw_gmtime(time_t t);
 int gw_rand(void);
 int gw_gethostbyname(struct hostent *ret, const char *name);
+void free_hostent(struct hostent *);

 /*
  * Make it harder to use these by mistake.
diff -ur -x '*.o' cvs/gateway/gwlib/socket.c work/gateway/gwlib/socket.c
--- cvs/gateway/gwlib/socket.c	2002-05-02 15:44:54.000000000 +0100
+++ work/gateway/gwlib/socket.c	2002-08-21 04:02:29.000000000 +0100
@@ -58,6 +58,7 @@
             goto error;
         }
         addr.sin_addr = *(struct in_addr *) hostinfo.h_addr;
+        free_hostent(&hostinfo);
     }

     reuse = 1;
@@ -116,7 +117,8 @@
     addr.sin_family = AF_INET;
     addr.sin_port = htons(port);
     addr.sin_addr = *(struct in_addr *) hostinfo.h_addr;
-
+    free_hostent(&hostinfo);
+
     if (our_port > 0 || (interface_name != NULL && strcmp(interface_name, "*") != 0))  {
         int reuse;

@@ -131,6 +133,7 @@
 		goto error;
 	    }
 	    o_addr.sin_addr = *(struct in_addr *) o_hostinfo.h_addr;
+            free_hostent(&o_hostinfo);
 	}

         reuse = 1;
@@ -305,6 +308,7 @@
             return -1;
         }
         sa.sin_addr = *(struct in_addr *) hostinfo.h_addr;
+        free_hostent(&hostinfo);
     }

     if (bind(s, (struct sockaddr *) &sa, (int) sizeof(sa)) == -1) {
@@ -335,6 +339,7 @@
             return NULL;
         }
         sa.sin_addr = *(struct in_addr *) h.h_addr_list[0];
+        free_hostent(&h);
     }

     return octstr_create_from_data((char *) &sa, sizeof(sa));
@@ -440,6 +445,7 @@
     } else {
         official_name = octstr_create(h.h_name);
 	official_ip = gw_netaddr_to_octstr(AF_INET, h.h_addr);
+        free_hostent(&h);
     }
 }