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

Ticket #1652 (closed defect: fixed)

Opened 10 years ago

Last modified 10 years ago

tango.net.Socket's definition of timeval is wrong for non-Windows x86-64

Reported by: Deewiant Assigned to: kris
Priority: critical Milestone:
Component: Tango Version: 0.99.9 Kai
Keywords: socket Cc:

Description

tango.net.Socket currently defines struct timeval as follows:

struct timeval
{
        int tv_sec; //seconds
        int tv_usec; //microseconds
}

This is not correct for 64-bit platforms, since the C definition uses time_t and suseconds_t, which are C longs. Both types should be c_long (from tango.stdc.config). Why isn't the struct defined in tango.stdc.sys.time anyway?

The toTimeval function is likewise incorrect, since it casts to uint:

static timeval toTimeval (TimeSpan time)
{
        timeval tv;
        tv.tv_sec = cast(uint) time.seconds;
        tv.tv_usec = cast(uint) time.micros % 1_000_000;
        return tv;
}

I don't think casting to unsigned types is the right thing to do here, but whether it is or not, the types should be c_long or c_ulong, not int or uint.

This has resulted in problems that were difficult to track down (LDC ticket 296), which is why I marked this as critical. It's an easy fix besides.

Attachments

timeval-fix.patch (2.8 kB) - added by Deewiant on 05/17/09 20:49:50.
Patch, untested on Windows or 32-bit
patch.diff (17.3 kB) - added by fawzi on 08/11/09 14:32:51.
corrected new patch with windows stuff moved to WsaSock??

Change History

05/15/09 15:47:28 changed by Deewiant

  • version changed from 0.99.8 Sean to trunk.

The problem is also present in trunk's tango.net.device.Berkeley: both the definition of struct timeval and the casts in SocketSet.select are incorrect.

05/15/09 17:43:05 changed by kris

  • owner changed from kris to fawzi.

Fawzi: where do you think this stuff should live?

05/17/09 20:02:32 changed by fawzi

tango.stdc.posix.sys.time

already defines timeval, on non windows that should be used.

05/17/09 20:49:50 changed by Deewiant

  • attachment timeval-fix.patch added.

Patch, untested on Windows or 32-bit

05/21/09 15:14:42 changed by fawzi

  • owner changed from fawzi to kris.

08/04/09 20:13:14 changed by larsivi

If it is tested for where it says it is untested, I will commit this.

08/04/09 21:13:31 changed by Deewiant

  • summary changed from tango.net.Socket's definition of timeval is wrong for x86-64 to tango.net.Socket's definition of timeval is wrong for non-Windows x86-64.

I just realized that this bug actually can't cause any trouble on Windows: even 64-bit Windows has a 32-bit c_long, so the int that is currently used is of the correct size.

Regardless, I tested the patch on 64-bit Windows Vista and the behaviour is the same with or without it, using the following test code:

import tango.time.Time;
import tango.net.device.Berkeley;

void main() {
	auto ss = new SocketSet(1);
	Berkeley sock;
	sock.open(AddressFamily.INET, SocketType.STREAM, ProtocolType.TCP);
	sock.bind(new IPv4Address("localhost"));
	sock.listen(1);
	ss.add(&sock);
	ss.select(ss, null, null, 0);
}

This is as expected. For Windows and 32-bit platforms, the patch should result in no changes.

08/05/09 14:23:32 changed by goshawk

  • keywords set to socket.
  • status changed from new to assigned.
  • owner changed from kris to goshawk.

I've just compiled a package with the patch in this ticket and i did:

goshawk@earth:~$ ldc socketserver.d
goshawk@earth:~$ ./socketserver
server replies 'hello'
goshawk@earth:~$

It works like a charm now :)

This patch should be merged with trunk.

To test out the patch in a compiled package look at https://launchpad.net/%7Ed-language-packagers/+archive/ppa/+sourcepub/691110/+listing-archive-extra

08/05/09 16:18:11 changed by fawzi

after a long chat with Deewiant it seems that the clean way to fix this (beside fixing the abvious int -> c_long is:

move windows c functions to sys.win32.WsaSock? (that should maybe be renamed to sys.windows.WinSock? remove the posix declarations and import the correct headers from stdc.posix.*

08/05/09 17:00:32 changed by goshawk

I've just tested the same patch on x86, here is the result:

goshawk@jupiter:~$ ldc socketserver.d
goshawk@jupiter:~$ ./socketserver
server replies 'hello'
goshawk@jupiter:~$ uname -a
Linux jupiter 2.6.28-14-server #47-Ubuntu SMP Sat Jul 25 01:18:34 UTC 2009 i686 GNU/Linux

Everything works fine :)

08/10/09 16:30:15 changed by fawzi

If someone tests the patch on windows I will check it in. The patch has still issues as Posix redeclares a bunch of stuff instead of importing it (mainly due to the use of socket_t as typedef). This probably should also be fixed.

08/11/09 00:52:50 changed by mwarning

I've tried to compile Tango with patch.diff on Windows XP and it failed because of the missing socket_t definition.
A quick fix resulted in more errors about the missing timeval definition...

08/11/09 05:34:49 changed by fawzi

Sorry, I had done a diff in a subdir, and the changes to WsaSock? were missing. Now they should be there.

08/11/09 14:32:51 changed by fawzi

  • attachment patch.diff added.

corrected new patch with windows stuff moved to WsaSock??

08/11/09 15:14:46 changed by fawzi

(In [4848]) fixing bad declaration of timeval, moved C windows socket stuff to WsaSock?, moving getaddrbyname declaration to newly created netdb module. refs #1652

08/11/09 15:24:39 changed by fawzi

the actual bug is now fixed, but the redeclaration of various c functions (with the possibility of using the wrong declaration on some OS) is still there. That should be addressed by the writers of these modules in my opinion.

I see two options to use the correct imports

  • change socket_t to an alias (but then it is not initialized to -1 as default)
  • cast socket_t to in when calling C functions

I would not change the declaration in the stdc modules to use socket_t because that would make porting C code more complicated, but if people strongly disagree I might reconsider.

08/11/09 15:49:12 changed by fawzi

(In [4849]) adding AddressFamily?, fixing exception in WsaSock?, refs #1652

08/12/09 10:09:07 changed by goshawk

  • status changed from assigned to closed.
  • resolution set to fixed.

02/27/10 10:49:54 changed by Deewiant

  • status changed from closed to reopened.
  • version changed from trunk to 0.99.9 Kai.
  • resolution deleted.
  • milestone deleted.

This is broken again in 0.99.9, and probably has been that way for who knows how long (I haven't been testing this lately): both tango.net.Socket and tango.net.device.Berkeley use int instead of c_long in struct timeval.

02/27/10 18:31:11 changed by kris

  • status changed from reopened to new.
  • owner changed from goshawk to kris.

02/27/10 18:32:07 changed by kris

  • status changed from new to closed.
  • resolution set to fixed.

(In [5386]) fixes #1652 :: tango.net.Socket's definition of timeval is wrong for non-Windows x86-64

thanks to Deewiant