Download Reference Manual
The Developer's Library for D
About Wiki Forums Source Search Contact

Ticket #563 (assigned defect)

Opened 1 year ago

Last modified 1 year ago

tango.net.Socket.d uses non reentrant and thread-unsafe C functionality

Reported by: Nietsnie Assigned to: kris (accepted)
Priority: critical Milestone: 1.0
Component: Core Functionality Version: 0.99 RC3 Xammy
Keywords: Cc:

Description

While going through the Socket.d code, I noticed some C library calls that will definitely make a fairly significant impact on using this library in a multi-threaded environment.

Here's the pieces I was able to find:

tango.net.Socket.IPv4Address.toAddrString() uses inet_ntoa (deprecated, uses a static buffer) should be inet_ntop, ie:

char[] toAddrString() {

char[16] buff; // 16 bytes is INET_ADDRSTRLEN
int AF_IENT = 2; // on linux...

return .toUtf8(inet_ntop(AF_INET, sin.sin_addr, buff.ptr, buff.sizeof)).dup;

}

Also:

tango.net.Socket.NetHost?.getHostByAddr uses gethostbyaddr which uses an internal static buffer:

The "new" way to do this is using getnameinfo ... However, big chunks of NetHost? will have to be rewritten as there's a lot of functionality that deals with hostents. I couldn't find a gethostbyaddr_r in my man pages, or you could probably use that.

Also:

tango.net.Socket.NetHost?.getHostByName uses gethostbyname which uses an internal static buffer.

The "new" way to do this is by using getaddrinfo, or gethostbyname_r would probably work.

Basically, any new InternetAddress? has a chance to have unknown data, as well as any call to toAddrInfo.

Attachments

SocketEnhanced.d (63.0 kB) - added by Nietsnie on 08/09/07 18:44:25.
Socket-getaddrinfo.patch (9.4 kB) - added by Nietsnie on 08/17/07 18:37:41.

Change History

08/09/07 18:44:25 changed by Nietsnie

  • attachment SocketEnhanced.d added.

08/09/07 18:47:27 changed by Nietsnie

I've added a Socket.d that has changed the functionality from inet_ntoa to inet_ntop, gethostbyname now ues getaddrinfo, gethostbyaddr uses getnameinfo.

Some caveats, getaddrinfo doesn't show aliases like gethostbyname does, at least on linux, but it'll resolve to multiple IPs, which is what I think you want.

Since these calls are apparently ANSI, and according to the MSDN exist in windows, it "should" work on Windows as well.

08/09/07 19:22:45 changed by Nietsnie

Oops, I had a typo in there

Line 1784 should be: return .toUtf8(inet_ntop(AddressFamily?.INET, &sin.sin_addr, buff.ptr, INET_ADDRSTRLEN)).dup;

not return .toUtf8(inet_ntop(AddressFamily?.INET, &sin, buff.ptr, INET_ADDRSTRLEN)).dup;

... I guess I could just make patch files instead of attaching the whole thing.

08/09/07 20:04:34 changed by sean

  • status changed from new to assigned.

No worries. I'd end up doing diffs to double-check either way :)

08/17/07 18:37:41 changed by Nietsnie

  • attachment Socket-getaddrinfo.patch added.

08/20/07 11:18:24 changed by Nietsnie

Looked into the windows missing symbols problems that Kris had, apparently getaddrinfo is supported in Windows XP and later (not 2000).

However, there is a workaround.

With Windows 2000, if you include the wspiapi.h, it has:

#define getaddrinfo WspiapiGetAddrInfo? #define getnameinfo WspiapiGetNameInfo? #define freeaddrinfo WspiapiFreeAddrInfo?

This will only work on IPv4. If you want IPv6 support, then installing the IPv6 preview update to Windows 2000 will give you the getaddrinfo symbols and you don't need the Wspiapi ones.

I don't have a windows system handy, so I'm unable to test these changes :/

09/05/07 14:57:58 changed by sean

  • milestone set to 0.99.2 RC5.

I'll give them a look next week. Assigning this task to 0.99.2.

09/12/07 13:58:05 changed by Nietsnie

I'm going to try and get this working tonight as I have a D environment setup on my Vista box now. So if you can't get around to it, I should be able to.

09/12/07 15:22:46 changed by sean

Some of the declarations really belong in a posix header (tango.stdc.posix.netdb), which I really need to create, but it would be great if you could perhaps update the patch for Socket. My schedule will be pretty busy for about the next two weeks, but I'll try to squeeze in Tango work whenever possible.

09/21/07 16:42:02 changed by Nietsnie

The "quick" fix to this (as Kris pointed out) is synchronize any calls to gethostbyname, gethostbyaddr, and inet_ntoa.

However, all of this functionality can be properly supported on windows, but would require to dynamically load the symbols (as you can't add a new .lib file to link with).

I think the dynamic linking way may be the best long-term solution, as there will be no synchronization if 30 sockets are trying to connect to 30 different hosts (ie, resolve one at a time would be slow).

inet_ntoa can be replaced by a manually written inet_ntop (pretty easy to do), which would be a solution to that problem without having to do anything super special.

09/29/07 15:51:21 changed by kris

  • owner changed from sean to kris.
  • status changed from assigned to new.
  • milestone changed from 0.99.2 RC5 to 1.0.

for now, I've added synchronized in the four relevant places

09/29/07 15:51:31 changed by kris

  • status changed from new to assigned.
Roadmap Timeline Tickets New Ticket Help / Guide About Trac
Login  |  
Trac dsource.org
RSS
Copyright © 2006-2008 Tango. All Rights Reserved. | Page Width: Static or Dynamic