Bug 1478 – Please use threadsafe functions in getHostByName

The following patch will change socket.d to use threadsafe functions for the gethostbyname call on non-windows platforms. Not doing this can lead to all sorts of ugly problems when using sockets in multi-threaded programs. The patch is originally for gdc's socket.d, but should be equally applicable to DMD. diff socket.d.broken socket.d 487c487,497 < hostent* he = gethostbyname(toStringz(name)); --- > version (Windows) > hostent* he = gethostbyname(toStringz(name)); > else > { > auto he = new hostent; > auto buffer=new char[512]; > int errno; > getHostByName_retry: // if we had extended do { } while { } this would not be necessary. > auto res = gethostbyname_r(toStringz(name), he, buffer.ptr, buffer.length, &he, &errno); > if (res == ERANGE) { buffer.length = buffer.length * 2; if (!he) he=new hostent; goto getHostByName_retry; } > }

Given that the _r versions of the functions are glibc specific, I'm much more inclined to synchronize the call. Additonally, this only protects one of the several non-threadsafe networking info api's. Comitted this diff: $ svn diff std/socket.d Index: std/socket.d =================================================================== --- std/socket.d (revision 358) +++ std/socket.d (working copy) @@ -453,7 +453,8 @@ */ bool getHostByName(string name) { - hostent* he = gethostbyname(toStringz(name)); + hostent* he; + synchronized he = gethostbyname(toStringz(name)); if(!he) return false; validHostent(he); @@ -468,7 +469,8 @@ bool getHostByAddr(uint addr) { uint x = htonl(addr); - hostent* he = gethostbyaddr(&x, 4, cast(int)AddressFamily.INET); + hostent* he; + synchronized he = gethostbyaddr(&x, 4, cast(int)AddressFamily.INET); if(!he) return false; validHostent(he); @@ -485,7 +487,8 @@ bool getHostByAddr(string addr) { uint x = inet_addr(std.string.toStringz(addr)); - hostent* he = gethostbyaddr(&x, 4, cast(int)AddressFamily.INET); + hostent* he; + synchronized he = gethostbyaddr(&x, 4, cast(int)AddressFamily.INET); if(!he) return false; validHostent(he);

downs: I saw your comments in scrollback on irc. Two comments: 1) thanks.. you're right about the need to globally synchronize and not per-class-instance synchronize. I've fixed that: - synchronized he = gethostbyname(toStringz(name)); + synchronized(this.classinfo) he = gethostbyname(toStringz(name)); 2) Regarding the use of static if, that would only work if there was sufficient configury mechanics to make sure that the _r versions were only included if they exist. All static if can see is what's been declared, not what actually is linkable. My main objection isn't really the declaration part of _r, but the very wierd usage of needing to grow that buffer and iterate many times. That'd likely end up more expensive than just synching. Thanks for catching #1. Later, Brad

[email protected] wrote: > http://d.puremagic.com/issues/show_bug.cgi?id=1478 > > > > > > ------- Comment #2 from [email protected] 2007-10-14 16:40 ------- > downs: I saw your comments in scrollback on irc. Two comments: > > 1) thanks.. you're right about the need to globally synchronize and not > per-class-instance synchronize. I've fixed that: > > - synchronized he = gethostbyname(toStringz(name)); > + synchronized(this.classinfo) he = gethostbyname(toStringz(name)); > > 2) Regarding the use of static if, that would only work if there was sufficient > configury mechanics to make sure that the _r versions were only included if > they exist. All static if can see is what's been declared, not what actually > is linkable. My main objection isn't really the declaration part of _r, but > the very wierd usage of needing to grow that buffer and iterate many times. > That'd likely end up more expensive than just synching. > > Thanks for catching #1. > > Later, > Brad > > > Regarding the growing part. On my system, the provided default buffer is sufficient, so it'd never need to regrow it. That's actually only there in case the buffer turns out to be insufficient on some systems. Here's an alternative. Since the required buffer size is unlikely to change much on the same system, how about storing the largest needed buffer size in a static class variable and update it on resize? This way, the resize would only take place the first time that the buffer is too small. Regarding the function being available on systems that don't actually have it; isn't that a bug in itself? Not sure on this one. Thanks for your reply though. --downs / feep ___________________________________________________________ Der fr

Fixed dmd 1.023