ws2_32: WSACleanup cleans up open sockets
Matt Durgavich
mattdurgavich at gmail.com
Tue Aug 25 14:18:52 CDT 2015
Hi all,
Here’s another attempt, with all of Ken’s suggestions in place.
-Matt
---
dlls/ws2_32/socket.c | 116 +++++++++++++++++++++++++++++++++++++++++++++--
dlls/ws2_32/tests/sock.c | 2 -
2 files changed, 113 insertions(+), 5 deletions(-)
diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c
index ca82ec9..465be2e 100644
--- a/dlls/ws2_32/socket.c
+++ b/dlls/ws2_32/socket.c
@@ -1429,6 +1429,15 @@ static int set_ipx_packettype(int sock, int ptype)
#endif
}
+#define TABLE_SIZE 256
+#define BUCKET_DEPTH 16
+static SOCKET socket_table[TABLE_SIZE][BUCKET_DEPTH];
+
+/* Cache support */
+static void add_to_table(SOCKET s);
+static BOOL remove_from_table(SOCKET s);
+static BOOL socket_in_table(SOCKET s);
+
/* ----------------------------------- API -----
*
* Init / cleanup / error checking.
@@ -1470,8 +1479,24 @@ int WINAPI WSAStartup(WORD wVersionRequested, LPWSADATA lpWSAData)
INT WINAPI WSACleanup(void)
{
if (num_startup) {
- num_startup--;
- TRACE("pending cleanups: %d\n", num_startup);
+ /* WS_closesocket needs num_startup to be non-zero, so decrement afterwards */
+ if (num_startup == 1) {
+ TRACE("cleaning up sockets");
+ int i, j;
+ for (i = 0; i < TABLE_SIZE; ++i) {
+ for (j = 0; j < BUCKET_DEPTH; ++j) {
+ SOCKET s = socket_table[i][j];
+ if (s) {
+ WS_closesocket(s);
+ }
+ }
+ }
+ num_startup = 0;
+ } else {
+ num_startup--;
+ TRACE("pending cleanups: %d\n", num_startup);
+ }
+
return 0;
}
SetLastError(WSANOTINITIALISED);
@@ -2595,6 +2620,11 @@ SOCKET WINAPI WS_accept(SOCKET s, struct WS_sockaddr *addr, int *addrlen32)
WS_closesocket(as);
return SOCKET_ERROR;
}
+ else
+ {
+ add_to_table(as);
+ }
+
TRACE("\taccepted %04lx\n", as);
return as;
}
@@ -2965,15 +2995,21 @@ int WINAPI WS_closesocket(SOCKET s)
int res = SOCKET_ERROR, fd;
if (num_startup)
{
+ BOOL success = FALSE;
fd = get_sock_fd(s, FILE_READ_DATA, NULL);
if (fd >= 0)
{
release_sock_fd(s, fd);
if (CloseHandle(SOCKET2HANDLE(s)))
res = 0;
+
+ success = remove_from_table(s);
}
- else
+
+ if (!success) {
SetLastError(WSAENOTSOCK);
+ res = SOCKET_ERROR;
+ }
}
else
SetLastError(WSANOTINITIALISED);
@@ -4987,6 +5023,11 @@ static int WS2_sendto( SOCKET s, LPWSABUF lpBuffers, DWORD dwBufferCount,
DWORD bytes_sent;
BOOL is_blocking;
+ if (!socket_in_table(s)) {
+ SetLastError(WSAENOTSOCK);
+ return SOCKET_ERROR;;
+ }
+
TRACE("socket %04lx, wsabuf %p, nbufs %d, flags %d, to %p, tolen %d, ovl %p, func %p\n",
s, lpBuffers, dwBufferCount, dwFlags,
to, tolen, lpOverlapped, lpCompletionRoutine);
@@ -6706,6 +6747,7 @@ SOCKET WINAPI WSASocketW(int af, int type, int protocol,
SERVER_END_REQ;
if (ret)
{
+ add_to_table(ret);
TRACE("\tcreated %04lx\n", ret );
if (ipxptype > 0)
set_ipx_packettype(ret, ipxptype);
@@ -8107,3 +8149,71 @@ INT WINAPI WSCEnumProtocols( LPINT protocols, LPWSAPROTOCOL_INFOW buffer, LPDWOR
return ret;
}
+
+/*****************/
+/* Cache support */
+
+static inline DWORD socket_to_index(SOCKET s)
+{
+ /* Hash to entry using Bernstein function */
+ DWORD h = 52812;
+ BYTE *b = (BYTE*)&s;
+ h = ((h << 5) + h) ^ b[0];
+ h = ((h << 5) + h) ^ b[1];
+ h = ((h << 5) + h) ^ b[2];
+ h = ((h << 5) + h) ^ b[3];
+ return h % TABLE_SIZE;
+}
+
+static void cache_update(SOCKET new_entry, SOCKET old_entry)
+{
+ int index, depth, tries;
+ LONG *bucket;
+
+ index = socket_to_index(new_entry == 0 ? old_entry : new_entry);
+ depth = 0;
+ bucket = (LONG*)&socket_table[index];
+ tries = 0;
+ do
+ {
+ LONG *slot = (LONG*)&bucket[depth++];
+ if (InterlockedCompareExchange(slot, new_entry, old_entry) == old_entry)
+ break;
+ if (depth == BUCKET_DEPTH)
+ {
+ ++tries;
+ depth = 0;
+ }
+
+ } while (tries < 3);
+ if (tries == 3)
+ {
+ ERR("Socket hash table bucket overflow. Resize buckets");
+ }
+}
+
+static void add_to_table(SOCKET s)
+{
+ cache_update(s, 0);
+}
+
+static BOOL remove_from_table(SOCKET s)
+{
+ if (!socket_in_table(s))
+ return FALSE;
+
+ cache_update(0, s);
+ return TRUE;
+}
+
+static inline BOOL socket_in_table(SOCKET s)
+{
+ int bucket, depth;
+ bucket = socket_to_index(s);
+ for (depth = 0; depth < TABLE_SIZE; ++depth) {
+ if (socket_table[bucket][depth] == s)
+ return TRUE;
+ }
+
+ return FALSE;
+}
diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c
index 2d14496..afa4063 100644
--- a/dlls/ws2_32/tests/sock.c
+++ b/dlls/ws2_32/tests/sock.c
@@ -1119,7 +1119,6 @@ static void test_WithWSAStartup(void)
ok(res == 0, "WSAStartup() failed unexpectedly: %d\n", res);
/* show that sockets are destroyed automatically after WSACleanup */
- todo_wine {
SetLastError(0xdeadbeef);
res = send(src, "TEST", 4, 0);
error = WSAGetLastError();
@@ -1131,7 +1130,6 @@ static void test_WithWSAStartup(void)
error = WSAGetLastError();
ok(res == SOCKET_ERROR, "closesocket should have failed\n");
ok(error == WSAENOTSOCK, "expected 10038, got %d\n", error);
- }
closesocket(src);
closesocket(dst);
--
2.3.2 (Apple Git-55)
> On Aug 25, 2015, at 1:44 PM, Matt Durgavich <mattdurgavich at gmail.com> wrote:
>
> Responses inline.
>
> Best,
> -Matt
>
> On Tuesday, Aug 25, 2015 at 12:46 AM, Ken Thomases <ken at codeweavers.com <mailto:ken at codeweavers.com>>, wrote:
> Thanks for continuing to work on this. By no means a thorough review:
>
> > On Aug 24, 2015, at 10:49 PM, Matt Durgavich <mattdurgavich at gmail.com> wrote:
>
> > +#define CACHE_SIZE 256
> > +#define CACHE_DEPTH 16
> > +static SOCKET socket_cache[CACHE_SIZE][CACHE_DEPTH];
> > +
> > +/* Cache support */
> > +static void add_to_cache(SOCKET s);
> > +static BOOL remove_from_cache(SOCKET s);
> > +static BOOL socket_in_cache(SOCKET s);
>
> I'm not sure this is a "cache", per se. It's a hash table, but naming issues are fairly minor.
>
>
> Let's go with socket_table
>
>
> > @@ -1470,8 +1479,24 @@ int WINAPI WSAStartup(WORD wVersionRequested, LPWSADATA lpWSAData)
> > INT WINAPI WSACleanup(void)
> > {
> > if (num_startup) {
> > - num_startup--;
> > - TRACE("pending cleanups: %d\n", num_startup);
> > + /* WS_closesocket needs num_startup to be non-zero, so decrement afterwards */
> > + if (num_startup - 1 == 0) {
>
> Why not "if (num_startup == 1)"?
>
> > +static void add_to_cache(SOCKET s)
> > +{
> > + int index, depth;
> > + SOCKET old;
> > + LONG *dest;
> > + index = socket_to_index(s) % CACHE_SIZE;
>
> If you always mod the result of socket_to_index() by CACHE_SIZE, why not do that in that function?
>
>
> Point taken.
>
>
> > + for (depth = 0; depth < CACHE_DEPTH; ++depth) {
> > + if (socket_cache[index][depth] == 0)
> > + break;
> > + }
> > +
> > + if (depth == CACHE_DEPTH) {
> > + ERR("Socket hash table collision\n");
>
> This should exit here or you access beyond the end of socket_cache[index] just below.
>
>
> Good catch.
>
>
> > + }
> > +
> > + dest = (PLONG)&socket_cache[index][depth];
>
> Don't use PLONG, just use LONG*.
>
>
> Ok
>
>
> > + old = InterlockedExchange(dest, s);
> > +
> > + if (old != 0) {
> > + ERR("Socket hash table internal corruption");
>
> Reporting corruption isn't right. You should use InterlockedCompareExchange() with 0 as the comparand to _avoid_ corruption. If it fails, loop back to the depth loop and try again. The only error possible should be filling the hash table bucket.
>
>
> Right. Makes sense. Will fix.
>
>
> > + }
> > +}
> > +
> > +static BOOL remove_from_cache(SOCKET s)
> > +{
> > + int index,depth;
> > + SOCKET old;
> > + LONG *dest;
> > + index = socket_to_index(s) % CACHE_SIZE;
> > + for (depth = 0; depth < CACHE_DEPTH; ++depth) {
> > + if (socket_cache[index][depth] == s)
> > + break;
> > + }
> > +
> > + if (depth == CACHE_DEPTH) {
> > + return FALSE;
> > + }
> > +
> > + dest = (PLONG)&socket_cache[index][depth];
> > + old = InterlockedExchange(dest, 0);
> > + return (old == s);
>
> Under what circumstances could old not equal s? If that can happen, then this should also use InterlockedCompareExchange(), otherwise you've removed some other socket.
>
>
> If I fork a child, I'll inherit their sockets right? So some paranoia in the remove is needed. Unless wine doesn't work that way. I'll write it as the analog to add.
>
>
> -Ken
>
>
> Thanks! I appreciate the feedback.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20150825/69bbdb8f/attachment-0001.html>
More information about the wine-devel
mailing list