Re: Possible race condition in gwlib/protected.c [version 1.1]
Kieran <[email protected]>
| Newsgroups | gmane.comp.mobile.kannel.devel |
|---|---|
| Message-ID | <[email protected]> |
On Wed, 21 Aug 2002, Stipe Tolj wrote: > > Nice catch. I like the patch too, and will vote for it if you stop > > using gw_native_malloc() and call the macro wrapper instead. some of > > us are using checking or even slow malloc from time to time, and its > > such a waste having a memory allocation checker and then opting out > > of using it. > > agreed with Oded here. Please resend the patch as attachement with the > abstracted malloc calls. > Fixed. As I said, I don't know the codebase yet. There hasn't been a response on the issue of using getaddrinfo() instead of gethostbyname(). I suggest this because getaddrinfo is supposed to be thread-safe. <http://www.opengroup.org/onlinepubs/007904975/functions/getaddrinfo.html> I'm happy to submit a patch if people are interested. Regards Kieran
patch1.1
(text/plain, 4.4 KB)
diff -ru -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 20:41:23.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_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_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_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 -ru -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 -ru -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);
}
}