After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 686786 - g_socket_get_available_bytes() returns wrong value on Windows/OSX
g_socket_get_available_bytes() returns wrong value on Windows/OSX
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: network
unspecified
Other All
: Normal major
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks: 610364
 
 
Reported: 2012-10-24 12:09 UTC by Sebastian Dröge (slomo)
Modified: 2013-08-31 15:44 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Return correct value for g_socket_get_available_bytes() on Windows and OSX (1.28 KB, patch)
2012-10-24 12:27 UTC, Sebastian Dröge (slomo)
none Details | Review
Return correct value for g_socket_get_available_bytes() on Windows and OSX (3.92 KB, patch)
2012-11-09 11:35 UTC, Sebastian Dröge (slomo)
none Details | Review
Return correct value for g_socket_get_available_bytes() on Windows and OSX (3.88 KB, patch)
2012-11-09 11:38 UTC, Sebastian Dröge (slomo)
none Details | Review

Description Sebastian Dröge (slomo) 2012-10-24 12:09:11 UTC
+++ This bug was initially created as a clone of Bug #610364 +++

Summary of that other bug:

> On Windows, "FIONREAD returns the amount of data that can be read in a single
> call to the recv function, which may not be the same as the total amount of
> data queued on the socket. If s is message oriented (for example, type
> SOCK_DGRAM), FIONREAD still returns the amount of pending data in the network
> buffer, however, the amount that can actually be read in a single call to the
> recv function is limited to the data size written in the send or sendto
> function call.", which means this is not really what we want.
>
> On OSX, ioctl(FIONREAD) returns the total buffersize.

Proposed change would be to use  WSAIoctl(FIONREAD) on Windows and getsockopt(SO_NREAD) on OSX, which returns the same value as on Linux (but arguably is a small change in behaviour, OTOH the old behaviour is broken IMHO).
Comment 1 Sebastian Dröge (slomo) 2012-10-24 12:27:12 UTC
Created attachment 227139 [details] [review]
Return correct value for g_socket_get_available_bytes() on Windows and OSX
Comment 2 Matthias Clasen 2012-10-25 10:20:23 UTC
Review of attachment 227139 [details] [review]:

::: gio/gsocket.c
@@ +2398,3 @@
+#else
+    if (WSAIoctl (socket->priv->fd, FIONREAD, NULL, 0, &avail, sizeof (avail), 0, 0) == SOCKET_ERROR)
+      return -1;

I'd prefer a cascade over a nested ifdef here:

#if defined(...)
#elif defined(...)
#else
#fi

Can't really comment on the win32 code myself, other than the general comment that it would be great to have a test case that verifies this. Do we ?
Comment 3 Sebastian Dröge (slomo) 2012-10-25 10:34:38 UTC
I'll change the preprocessor directives before committing or when another change is required :)
I should probably add that I didn't even test to compile the Windows/OSX changes because I don't have Windows/OSX machines here.

A testcase would be nice, yes, but should probably be done by someone who can actually test this.
Comment 4 Dan Winship 2012-10-27 17:32:02 UTC
Technically the Windows/OS X behavior matches the g_socket_get_available_bytes() documentation better, but the Linux behavior is clearly more useful, so... patch is good but you should also fix the documentation to describe the actual (new) behavior better.

(In reply to comment #3)
> A testcase would be nice, yes, but should probably be done by someone who can
> actually test this.

You can test it under unix at least and then someone else can run it under the other OSes.
Comment 5 Sebastian Dröge (slomo) 2012-11-09 11:35:53 UTC
Created attachment 228553 [details] [review]
Return correct value for g_socket_get_available_bytes() on Windows and OSX
Comment 6 Sebastian Dröge (slomo) 2012-11-09 11:38:14 UTC
Created attachment 228555 [details] [review]
Return correct value for g_socket_get_available_bytes() on Windows and OSX
Comment 7 Sebastian Dröge (slomo) 2012-11-09 11:39:59 UTC
This should address all the comments and also adds a test for this.
Comment 8 Dan Winship 2012-11-11 18:49:20 UTC
this doesn't even compile on windows...
Comment 9 Dan Winship 2012-11-11 19:22:44 UTC
And it doesn't do what it's supposed to either. The WSAIoctl docs say:

    If the socket passed in the s parameter is message oriented (for
    example, type SOCK_DGRAM), FIONREAD returns the reports the total
    number of bytes available to read, not the size of the first
    datagram (message) queued on the socket.

ie, exactly the same as ioctlsocket.

(Note that it does *appear* to work under Wine [with either WSAIoctl or ioctlsocket], but that's because Wine just proxies the winsock call to the corresponding linux ioctl, so it gets the linux behavior.)

I've reverted the patch for now, although it made it into 2.34.2 (which means that 2.34.2 doesn't compile on Windows, so you might want to do a 2.34.2.1, Matthias).
Comment 10 Sebastian Dröge (slomo) 2012-11-12 08:14:37 UTC
(In reply to comment #8)
> this doesn't even compile on windows...

Yeah as said above I didn't try to compile it on Windows/OSX because I have no access to any of those. I'm a bit surprised that it immediately went to the stable branch though


Will try to get the correct way of doing it on Windows
Comment 11 Pierre Boutillier 2012-11-12 12:39:10 UTC
2.34.2 does not compiles on MacOS (10.8) either.

I don't know glib but it seems obvious why, doesn't it ?

Under MacOS, G_OS_WIN32 is not defined but SO_NREAD is, right ?

> gssize
> g_socket_get_available_bytes (GSocket *socket)
> {
> #ifndef G_OS_WIN32
>   gulong avail = 0;
> #else
>   gint avail = 0;
>   gsize avail_len = sizeof (avail);
> #endif
So avail_len is not defined
>   g_return_val_if_fail (G_IS_SOCKET (socket), -1);

> #if defined(G_OS_WIN32)
>   if (WSAIoctl (socket->priv->fd, FIONREAD, NULL, 0, &avail, sizeof (avail), 0, 0) == SOCKET_ERROR)
>     return -1;
> #elif defined(SO_NREAD)
>   if (getsockopt (socket->priv->fd, SOL_SOCKET, SO_NREAD, &avail, &avail_len) < 0)
>     return -1;
> #else
but used.
>   if (ioctl (socket->priv->fd, FIONREAD, &avail) < 0)
>     return -1;
> #endif

>   return avail;
> }
Comment 12 Ryan Schmidt 2012-11-14 05:29:30 UTC
Here's the patch I used in MacPorts to build glib 2.34.2:

https://trac.macports.org/browser/trunk/dports/devel/glib2/files/patch-gio_gsocket.c.diff?rev=99676
Comment 13 Dan Winship 2012-11-15 13:42:28 UTC
(In reply to comment #10)
> Will try to get the correct way of doing it on Windows

It appears that the only way to do it is to do a recv with MSG_PEEK into a 64k buffer. (64k being the largest possible size for a single UDP packet, so you know that's enough that you won't end up dropping any data.)

Since you don't actually care about the contents of the buffer after the recv, you could use a single global buffer (rather than needing one per thread). It would probably make sense to malloc it when needed (with g_once) rather than having a static 64k buffer. And of course, this is only needed for UDP sockets; FIONREAD will do the expected thing for TCP sockets.

This is gross, and the docs should probably note that this function will do gross things on some platforms with UDP sockets, with a recommendation that if you know a (reasonable) upper bound on the packet size, you should just recv into a buffer of that size rather than using get_available_bytes().
Comment 14 Daniel Macks 2012-11-18 16:56:52 UTC
Comment #12 allows me to compile on OS X 10.6, but the /socket/datagram_get_available self-test fails:

  /socket/datagram_get_available:                                      **
GLib-GIO:ERROR:socket.c:853:test_datagram_get_available: assertion failed (g_socket_get_available_bytes (server) == sizeof (data)): (0 == 17)
FAIL
GTester: last random seed: R02S0ce15129c62049d5cab3ef35fae5f744

Same result whether I use 32-bit or 64-bit mode (-arch i386 or -arch x86_64 compiler flags, respectively).
Comment 15 Dan Winship 2013-08-31 15:44:40 UTC
Pushed an updated patch that's tested on linux, os x, and cygwin