You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Justin Erenkrantz <je...@ebuilt.com> on 2001/07/20 04:36:55 UTC
[PATCH] Add reentrant gethostbyname call
During some testing with flood (~30 threads), we ran into a stack
corruption error in our code. We tracked it down to the fact that we
were using a non-reentrant version of gethostbyname. This patch lets
us call the reentrant version of gethostbyname and now we haven't
been able to recreate the segfault.
The only possible optimization would be the size of the temporary
buffer (currently 256). Roy mentioned that the address array in
hostent has a maximum of 10 entries. If so, the size of the
structure plus the maximum size of the array (10 entries) may be
sufficient. If we send in a too small buffer, we should
receive ERANGE.
Any reason we shouldn't commit? -- justin
Index: network_io/unix/sa_common.c
===================================================================
RCS file: /home/cvs/apr/network_io/unix/sa_common.c,v
retrieving revision 1.34
diff -u -r1.34 sa_common.c
--- network_io/unix/sa_common.c 2001/05/02 02:54:11 1.34
+++ network_io/unix/sa_common.c 2001/07/20 02:29:30
@@ -380,6 +380,11 @@
struct hostent *hp;
apr_sockaddr_t *cursa;
int curaddr;
+#if APR_HAS_THREADS
+ char tmp[256];
+ int hosterror;
+ struct hostent hs;
+#endif
if (family == APR_UNSPEC) {
family = APR_INET; /* we don't support IPv6 here */
@@ -395,11 +400,17 @@
}
else {
#endif
+#if APR_HAS_THREADS
+ hp = gethostbyname_r(hostname, &hs, tmp, 255, &hosterror);
+#else
hp = gethostbyname(hostname);
+#endif
if (!hp) {
#ifdef WIN32
apr_get_netos_error();
+#elif APR_HAS_THREADS
+ return (hosterror + APR_OS_START_SYSERR);
#else
return (h_errno + APR_OS_START_SYSERR);
#endif
[PATCH] #2 was Re: [PATCH] Add reentrant gethostbyname call
Posted by Justin Erenkrantz <je...@ebuilt.com>.
Improvements and notes since my original posting:
- Use autoconf detection to try to eliminate chances where we may
not need gethostbyname_r
- Fixed one long line that bugged me.
- Bump up buffersize to 512 since it may be larger than I thought
(need to give Roy more Dr. Pepper). I was using it with 256
before and didn't see any problem, but 512 is a bit more
conservative. Hopefully if the user runs out of range, whatever
calls this function will notice the error (ERANGE) and report
it to us and we can bump the #define if need be.
- AFAICT, gethostbyname_r is *not* defined in POSIX 1003.1c-1995, so
we can't use _POSIX_THREAD_SAFE_FUNCTIONS as we can for readdir_r.
- Solaris 8, which has getipnodebyname and getaddrinfo, also has
IPV6, so it takes a completely different code path. We were
using Solaris 7 today, so that may be why I haven't seen problems
before now.
- Roy mentioned that the gethostbyname_r in Solaris has different
semantics than POSIX. Well, I can't/don't have access to any
platforms that uses the different prototype. So, if there is
another platform out there that has gethostbyname_r with a
different prototype, you'll have to add in the tweaks or submit
patches. Sorry.
I'll plan on committing in the morning unless I receive any
objections. I wonder how many more of these non-threadsafe
functions we're using with threads. =( -- justin
Index: configure.in
===================================================================
RCS file: /home/cvs/apr/configure.in,v
retrieving revision 1.339
diff -u -r1.339 configure.in
--- configure.in 2001/07/14 00:13:36 1.339
+++ configure.in 2001/07/20 05:56:47
@@ -326,9 +326,12 @@
fi
ac_cv_define_READDIR_IS_THREAD_SAFE=no
+ac_cv_define_GETHOSTBYNAME_IS_THREAD_SAFE=no
if test "$threads" = "1"; then
echo "APR will use threads"
AC_CHECK_LIB(c_r, readdir, AC_DEFINE(READDIR_IS_THREAD_SAFE))
+ AC_CHECK_LIB(c_r, gethostbyname, AC_DEFINE(GETHOSTBYNAME_IS_THREAD_SAFE))
+ AC_CHECK_FUNCS(gethostbyname_r)
else
echo "APR will be non-threaded"
fi
Index: acconfig.h
===================================================================
RCS file: /home/cvs/apr/acconfig.h,v
retrieving revision 1.46
diff -u -r1.46 acconfig.h
--- acconfig.h 2001/07/02 18:54:05 1.46
+++ acconfig.h 2001/07/20 05:56:47
@@ -23,6 +23,7 @@
#undef USE_PTHREAD_SERIALIZE
#undef READDIR_IS_THREAD_SAFE
+#undef GETHOSTBYNAME_IS_THREAD_SAFE
#undef NEED_RLIM_T
#undef USEBCOPY
Index: network_io/unix/sa_common.c
===================================================================
RCS file: /home/cvs/apr/network_io/unix/sa_common.c,v
retrieving revision 1.34
diff -u -r1.34 sa_common.c
--- network_io/unix/sa_common.c 2001/05/02 02:54:11 1.34
+++ network_io/unix/sa_common.c 2001/07/20 05:56:47
@@ -77,6 +77,15 @@
#define SET_H_ERRNO(newval) h_errno = (newval)
#endif
+#if APR_HAS_THREADS && !defined(GETHOSTBYNAME_IS_THREAD_SAFE) && \
+ defined(HAVE_GETHOSTBYNAME_R)
+/* This is the maximum size that may be returned from the reentrant
+ * gethostbyname_r function. If the system tries to use more, it
+ * should return ERANGE.
+ */
+#define GETHOSTBYNAME_BUFLEN 512
+#endif
+
APR_DECLARE(apr_status_t) apr_sockaddr_port_set(apr_sockaddr_t *sockaddr,
apr_port_t port)
{
@@ -380,6 +389,12 @@
struct hostent *hp;
apr_sockaddr_t *cursa;
int curaddr;
+#if APR_HAS_THREADS && !defined(GETHOSTBYNAME_IS_THREAD_SAFE) && \
+ defined(HAVE_GETHOSTBYNAME_R)
+ char tmp[GETHOSTBYNAME_BUFLEN];
+ int hosterror;
+ struct hostent hs;
+#endif
if (family == APR_UNSPEC) {
family = APR_INET; /* we don't support IPv6 here */
@@ -395,11 +410,22 @@
}
else {
#endif
+#if APR_HAS_THREADS && !defined(GETHOSTBYNAME_IS_THREAD_SAFE) && \
+ defined(HAVE_GETHOSTBYNAME_R)
+ hp = gethostbyname_r(hostname, &hs, tmp, GETHOSTBYNAME_BUFLEN - 1,
+ &hosterror);
+#else
hp = gethostbyname(hostname);
+#endif
if (!hp) {
#ifdef WIN32
apr_get_netos_error();
+#elif APR_HAS_THREADS && !defined(GETHOSTBYNAME_IS_THREAD_SAFE) && \
+ defined(HAVE_GETHOSTBYNAME_R)
+ /* If you see ERANGE, that means GETHOSBYNAME_BUFLEN needs to be
+ * bumped. */
+ return (hosterror + APR_OS_START_SYSERR);
#else
return (h_errno + APR_OS_START_SYSERR);
#endif
@@ -412,7 +438,7 @@
while (hp->h_addr_list[curaddr]) {
cursa->next = apr_pcalloc(p, sizeof(apr_sockaddr_t));
cursa = cursa->next;
- save_addrinfo(p, cursa, *(struct in_addr *)hp->h_addr_list[curaddr],
+ save_addrinfo(p, cursa, *(struct in_addr *)hp->h_addr_list[curaddr],
port);
++curaddr;
}
Re: [PATCH] Add reentrant gethostbyname call
Posted by Jeff Trawick <tr...@attglobal.net>.
Dale Ghent <da...@elemental.org> writes:
> In Solaris 8, it is preferable to use the IPv6-derived getaddrinfo() (as
> we already do)
FYI... On all platforms it is preferable to use getaddrinfo().
getipnodebyname() was in some earlier IPv6 basic API RFC but is not in
2553bis.
--
Jeff Trawick | trawick@attglobal.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [PATCH] Add reentrant gethostbyname call
Posted by Dale Ghent <da...@elemental.org>.
On Thu, 19 Jul 2001, Roy T. Fielding wrote:
| The test needs to be a bit more complicated than that,
| since Solaris has different definitions of gethostbyname_r defined based
| on the POSIX level. Bugger, it looks like the Solaris man page is
| recommending against that function anyway, in favor of getipnodebyname()
| because the old ones won't work with IPv6. What's the likelihood
| that it will be portable? Perhaps we should write a test for each
| and use the best one found.
getipnodebyname() is new as of Solaris 8.
In Solaris 8, it is preferable to use the IPv6-derived getaddrinfo() (as
we already do)
In < Solaris 8, we should use gethostby{addr|name}_r() when threads are
used (which is the default.)
On the issue of it being overhead... I think the point's moot since we're
wanting to do a DNS lookup anyways. True, in httpd, that may matter. But
for other apps (samba, perhaps), the benefit of a resolver outwieghs any
overhead it may cause.
So let's finish this out with a rounded-out resolver interface and strike
an item of the httpd STATUS list. I know that the icecast streaming mp3
server deals with the various implementations of gethostby*_r, and
eventhough it's GPL, it may be worth a look at for ideas.
/dale
Re: [PATCH] Add reentrant gethostbyname call
Posted by "Roy T. Fielding" <fi...@ebuilt.com>.
> The only possible optimization would be the size of the temporary
> buffer (currently 256). Roy mentioned that the address array in
> hostent has a maximum of 10 entries. If so, the size of the
> structure plus the maximum size of the array (10 entries) may be
> sufficient. If we send in a too small buffer, we should
> receive ERANGE.
Er, did I say that? Not enough sugar, I guess. I meant that normal DNS
responses were effectively limited to 10, due to a limitation in
some other client operating system or web browser, such that there
wasn't much point in having a round-robin table with more than ten
entries. I'm not sure what the actual receive limit is on Solaris.
Also, I think the buffer might be used for both hostent and the two lists
of names/aliases, which would make the memory need much larger than 256.
It should be a symbol, in any case, so that people can redefine it
per platform.
The test needs to be a bit more complicated than that,
since Solaris has different definitions of gethostbyname_r defined based
on the POSIX level. Bugger, it looks like the Solaris man page is
recommending against that function anyway, in favor of getipnodebyname()
because the old ones won't work with IPv6. What's the likelihood
that it will be portable? Perhaps we should write a test for each
and use the best one found.
....Roy
Re: [PATCH] Add reentrant gethostbyname call
Posted by Ryan Bloom <rb...@covalent.net>.
On Thursday 19 July 2001 19:54, Ian Holsman wrote:
> Justin Erenkrantz wrote:
> >During some testing with flood (~30 threads), we ran into a stack
> >corruption error in our code. We tracked it down to the fact that
> > we were using a non-reentrant version of gethostbyname. This
> > patch lets us call the reentrant version of gethostbyname and now
> > we haven't been able to recreate the segfault.
> >
> >The only possible optimization would be the size of the temporary
> >buffer (currently 256). Roy mentioned that the address array in
> >hostent has a maximum of 10 entries. If so, the size of the
> >structure plus the maximum size of the array (10 entries) may be
> >sufficient. If we send in a too small buffer, we should
> >receive ERANGE.
>
> Is there a reason why we shouldn't just use the re-entrant versions
> where it is available?
It can be overhead that we don't need, it all depends on how the
re-entrant version is implemented.
> >Any reason we shouldn't commit? -- justin
You can't check for threads that way. You need to use the same basic
logic that we use in dir.c to check for readdir_r.
Ryan
Re: [PATCH] Add reentrant gethostbyname call
Posted by Ian Holsman <ia...@cnet.com>.
Justin Erenkrantz wrote:
>During some testing with flood (~30 threads), we ran into a stack
>corruption error in our code. We tracked it down to the fact that we
>were using a non-reentrant version of gethostbyname. This patch lets
>us call the reentrant version of gethostbyname and now we haven't
>been able to recreate the segfault.
>
>The only possible optimization would be the size of the temporary
>buffer (currently 256). Roy mentioned that the address array in
>hostent has a maximum of 10 entries. If so, the size of the
>structure plus the maximum size of the array (10 entries) may be
>sufficient. If we send in a too small buffer, we should
>receive ERANGE.
>
Is there a reason why we shouldn't just use the re-entrant versions
where it is available?
also you can't use APR_HAS_THREADS to check,
as someone could have threads turned on in BSD, which
doesnt have a gethostbyname_r function
(at least acording to cvs.apache.org's man pages)
..Ian
>
>
>Any reason we shouldn't commit? -- justin
>
>Index: network_io/unix/sa_common.c
>===================================================================
>RCS file: /home/cvs/apr/network_io/unix/sa_common.c,v
>retrieving revision 1.34
>diff -u -r1.34 sa_common.c
>--- network_io/unix/sa_common.c 2001/05/02 02:54:11 1.34
>+++ network_io/unix/sa_common.c 2001/07/20 02:29:30
>@@ -380,6 +380,11 @@
> struct hostent *hp;
> apr_sockaddr_t *cursa;
> int curaddr;
>+#if APR_HAS_THREADS
>+ char tmp[256];
>+ int hosterror;
>+ struct hostent hs;
>+#endif
>
> if (family == APR_UNSPEC) {
> family = APR_INET; /* we don't support IPv6 here */
>@@ -395,11 +400,17 @@
> }
> else {
> #endif
>+#if APR_HAS_THREADS
>+ hp = gethostbyname_r(hostname, &hs, tmp, 255, &hosterror);
>+#else
> hp = gethostbyname(hostname);
>+#endif
>
> if (!hp) {
> #ifdef WIN32
> apr_get_netos_error();
>+#elif APR_HAS_THREADS
>+ return (hosterror + APR_OS_START_SYSERR);
> #else
> return (h_errno + APR_OS_START_SYSERR);
> #endif
>