Re: ws2_32: WSACleanup cleans up open sockets (OSX only)
Hi, Thanks for contributing to Wine. I've commented on the patch in-line below: On Jul 31, 2015, at 10:19 PM, Matt Durgavich <mattdurgavich(a)gmail.com> wrote:
+static void close_open_sockets() { +#ifdef HAVE_LIBPROC_H + int pid = getpid(); + int bufferSizeNeeded = proc_pidinfo(pid, PROC_PIDLISTFDS, 0, NULL, 0);
This function isn't documented and the header which declares it says it's SPI rather than API and can change at any time. Also, is this the right approach generally? This is going to close all Unix sockets opened by the process, not just Win32 sockets opened by ws2_32. Is it safe/proper for WSACleanup to close sockets that it didn't open? Isn't it possible that other parts of Wine could open Unix sockets for other reasons? Or that system libraries or frameworks might do so to carry out requests made by Wine?
+ if (bufferSizeNeeded > 0) { + struct proc_fdinfo *infos = (struct proc_fdinfo*)malloc(bufferSizeNeeded); + int ret = proc_pidinfo(pid, PROC_PIDLISTFDS, 0, infos, bufferSizeNeeded); + int numfds = bufferSizeNeeded / PROC_PIDLISTFD_SIZE;
Shouldn't you use ret / PROC_PIDLISTFD_SIZE (assuming no error)?
+ if (ret >= 0) { + for (int i = 0; i < numfds; ++i ) {
I don't think declaring the loop variable in the loop is allowed by Wine's coding standards.
+ int32_t fd = infos[i].proc_fd; + uint32_t type = infos[i].proc_fdtype; + if (type == PROX_FDTYPE_SOCKET) { + TRACE("Closing socket with descriptor %d\n", fd); + close(fd); + } + } + } + free(infos); + } +#else + FIXME("stub") +#endif +}
/*********************************************************************** * WSACleanup (WS2_32.116) @@ -1458,8 +1490,14 @@ INT WINAPI WSACleanup(void) if (num_startup) { num_startup--; TRACE("pending cleanups: %d\n", num_startup); + if (num_startup == 0) { + TRACE("cleaning up open sockets"); + close_open_sockets(); + } return 0; } + + /* Close all sockets */
What's this last comment about? Did you mean to put it above?
SetLastError(WSANOTINITIALISED); return SOCKET_ERROR; }
-Ken
On Jul 31, 2015, at 11:31 PM, Ken Thomases <ken(a)codeweavers.com> wrote:
Hi,
Thanks for contributing to Wine. I've commented on the patch in-line below:
On Jul 31, 2015, at 10:19 PM, Matt Durgavich <mattdurgavich(a)gmail.com> wrote:
+static void close_open_sockets() { +#ifdef HAVE_LIBPROC_H + int pid = getpid(); + int bufferSizeNeeded = proc_pidinfo(pid, PROC_PIDLISTFDS, 0, NULL, 0);
This function isn't documented and the header which declares it says it's SPI rather than API and can change at any time. Well then, we’re kinda screwed if they do change it, because we already use that SPI. (In fact, it’s my fault. ;) In particular, we use it in iphlpapi to figure out which process owns a particular socket. (@Matt: this also means the check for <libproc.h> you added to configure.ac is redundant, because we already check for it. You can delete it.)
Also, is this the right approach generally? This is going to close all Unix sockets opened by the process, not just Win32 sockets opened by ws2_32. It won’t even (properly) close all (or any, really) of the winsock sockets. That’s because winsock sockets in Wine, like on NT, are NT-style object handles. That’s really what you have to find and close. Also, even if you try to map the FDs back to their corresponding Wine handles, you still won’t necessarily get them all, because the wineserver manages FDs (including sockets, I believe) on Wine’s behalf; they’re only present in the process if Wine bothers to go to the wineserver to get them (cf. dlls/ntdll/server.c).
So, I suggest enumerating all the socket handles in the process (e.g. by enumerating all handles in this process using toolhelp or psapi, then filtering out the ones that aren’t sockets), and closing those. Or, maybe, having winsock keep track of sockets that it opens (e.g. returned from WSASocket()), then having WSACleanup() close all the ones it knows about. The great thing about doing it either of these ways is that it’ll work elsewhere, too—not just on Mac OS. The second option I gave might be preferable, because e.g. someone could DuplicateHandle() a winsock handle into another process, and WSACleanup() might not close it when it runs. (Someone should write a test for that.)
Is it safe/proper for WSACleanup to close sockets that it didn't open? Isn't it possible that other parts of Wine could open Unix sockets for other reasons? It is, and in fact, they do. For one thing, Wine programs use Unix-domain (PF_UNIX) sockets to talk to wineserver.
Chip
Hi all and sorry, replying from mobile phone so I can't quote things right. There are a few tests covering this so any patch fixing this problem will require reviewing the tests too. I tried to fix this last year, the discussion and patch can be found at [1]. There is also a real bug related to this problem (18670 and 26031). 1- http://marc.info/?t=138834097400003&r=1&w=2 Best wishes, Bruno On Sunday, August 2, 2015, Charles Davis <cdavis5x(a)gmail.com> wrote:
On Jul 31, 2015, at 11:31 PM, Ken Thomases <ken(a)codeweavers.com <javascript:;>> wrote:
Hi,
Thanks for contributing to Wine. I've commented on the patch in-line below:
On Jul 31, 2015, at 10:19 PM, Matt Durgavich <mattdurgavich(a)gmail.com <javascript:;>> wrote:
+static void close_open_sockets() { +#ifdef HAVE_LIBPROC_H + int pid = getpid(); + int bufferSizeNeeded = proc_pidinfo(pid, PROC_PIDLISTFDS, 0, NULL, 0);
This function isn't documented and the header which declares it says it's SPI rather than API and can change at any time. Well then, we’re kinda screwed if they do change it, because we already use that SPI. (In fact, it’s my fault. ;) In particular, we use it in iphlpapi to figure out which process owns a particular socket. (@Matt: this also means the check for <libproc.h> you added to configure.ac is redundant, because we already check for it. You can delete it.)
Also, is this the right approach generally? This is going to close all Unix sockets opened by the process, not just Win32 sockets opened by ws2_32. It won’t even (properly) close all (or any, really) of the winsock sockets. That’s because winsock sockets in Wine, like on NT, are NT-style object handles. That’s really what you have to find and close. Also, even if you try to map the FDs back to their corresponding Wine handles, you still won’t necessarily get them all, because the wineserver manages FDs (including sockets, I believe) on Wine’s behalf; they’re only present in the process if Wine bothers to go to the wineserver to get them (cf. dlls/ntdll/server.c).
So, I suggest enumerating all the socket handles in the process (e.g. by enumerating all handles in this process using toolhelp or psapi, then filtering out the ones that aren’t sockets), and closing those. Or, maybe, having winsock keep track of sockets that it opens (e.g. returned from WSASocket()), then having WSACleanup() close all the ones it knows about. The great thing about doing it either of these ways is that it’ll work elsewhere, too—not just on Mac OS. The second option I gave might be preferable, because e.g. someone could DuplicateHandle() a winsock handle into another process, and WSACleanup() might not close it when it runs. (Someone should write a test for that.)
Is it safe/proper for WSACleanup to close sockets that it didn't open? Isn't it possible that other parts of Wine could open Unix sockets for other reasons? It is, and in fact, they do. For one thing, Wine programs use Unix-domain (PF_UNIX) sockets to talk to wineserver.
Chip
Thanks for all the great input. I'm an experienced developer but a newbie at wine dev, so the input is greatly appreciated. I'll take a stab at iterating on Bruno's last patch to leverage the server's handles. -Matt On Sun, Aug 2, 2015 at 11:13 AM, Bruno Jesus <00cpxxx(a)gmail.com> wrote:
Hi all and sorry, replying from mobile phone so I can't quote things right.
There are a few tests covering this so any patch fixing this problem will require reviewing the tests too.
I tried to fix this last year, the discussion and patch can be found at [1]. There is also a real bug related to this problem (18670 and 26031).
1- http://marc.info/?t=138834097400003&r=1&w=2
Best wishes, Bruno
On Sunday, August 2, 2015, Charles Davis <cdavis5x(a)gmail.com> wrote:
On Jul 31, 2015, at 11:31 PM, Ken Thomases <ken(a)codeweavers.com> wrote:
Hi,
Thanks for contributing to Wine. I've commented on the patch in-line below:
On Jul 31, 2015, at 10:19 PM, Matt Durgavich <mattdurgavich(a)gmail.com> wrote:
+static void close_open_sockets() { +#ifdef HAVE_LIBPROC_H + int pid = getpid(); + int bufferSizeNeeded = proc_pidinfo(pid, PROC_PIDLISTFDS, 0, NULL, 0);
This function isn't documented and the header which declares it says it's SPI rather than API and can change at any time. Well then, we’re kinda screwed if they do change it, because we already use that SPI. (In fact, it’s my fault. ;) In particular, we use it in iphlpapi to figure out which process owns a particular socket. (@Matt: this also means the check for <libproc.h> you added to configure.ac is redundant, because we already check for it. You can delete it.)
Also, is this the right approach generally? This is going to close all Unix sockets opened by the process, not just Win32 sockets opened by ws2_32. It won’t even (properly) close all (or any, really) of the winsock sockets. That’s because winsock sockets in Wine, like on NT, are NT-style object handles. That’s really what you have to find and close. Also, even if you try to map the FDs back to their corresponding Wine handles, you still won’t necessarily get them all, because the wineserver manages FDs (including sockets, I believe) on Wine’s behalf; they’re only present in the process if Wine bothers to go to the wineserver to get them (cf. dlls/ntdll/server.c).
So, I suggest enumerating all the socket handles in the process (e.g. by enumerating all handles in this process using toolhelp or psapi, then filtering out the ones that aren’t sockets), and closing those. Or, maybe, having winsock keep track of sockets that it opens (e.g. returned from WSASocket()), then having WSACleanup() close all the ones it knows about. The great thing about doing it either of these ways is that it’ll work elsewhere, too—not just on Mac OS. The second option I gave might be preferable, because e.g. someone could DuplicateHandle() a winsock handle into another process, and WSACleanup() might not close it when it runs. (Someone should write a test for that.)
Is it safe/proper for WSACleanup to close sockets that it didn't open? Isn't it possible that other parts of Wine could open Unix sockets for other reasons? It is, and in fact, they do. For one thing, Wine programs use Unix-domain (PF_UNIX) sockets to talk to wineserver.
Chip
Here’s my attempt at Bruno’s patch with a fast cache. Comments welcome. -Matt diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c index ca82ec9..65925ba 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 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); + /* ----------------------------------- 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 == 0) { + TRACE("cleaning up sockets"); + int i, j; + for (i = 0; i < CACHE_SIZE; ++i) { + for (j = 0; j < CACHE_DEPTH; ++j) { + SOCKET s = socket_cache[i][j]; + if (s) { + WS_closesocket(s); + } + } + } + num_startup--; + } 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_cache(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_cache(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_cache(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_cache(ret); TRACE("\tcreated %04lx\n", ret ); if (ipxptype > 0) set_ipx_packettype(ret, ipxptype); @@ -8107,3 +8149,73 @@ 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; +} + +static void add_to_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] == 0) + break; + } + + if (depth == CACHE_DEPTH) { + ERR("Socket hash table collision\n"); + } + + dest = (PLONG)&socket_cache[index][depth]; + old = InterlockedExchange(dest, s); + + if (old != 0) { + ERR("Socket hash table internal corruption"); + } +} + +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); +} + +static inline BOOL socket_in_cache(SOCKET s) +{ + int index, depth; + index = socket_to_index(s) % CACHE_SIZE; + for (depth = 0; depth < CACHE_SIZE; ++depth) { + if (socket_cache[index][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);
Thanks for continuing to work on this. By no means a thorough review:
On Aug 24, 2015, at 10:49 PM, Matt Durgavich <mattdurgavich(a)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.
@@ -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?
+ 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.
+ } + + dest = (PLONG)&socket_cache[index][depth];
Don't use PLONG, just use LONG*.
+ 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.
+ } +} + +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. -Ken
On 25 August 2015 at 06:46, Ken Thomases <ken(a)codeweavers.com> wrote:
On Aug 24, 2015, at 10:49 PM, Matt Durgavich <mattdurgavich(a)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.
If the idea is just to keep track of all the sockets you give out you want neither of those of course, but just something like a handle table or a list.
On Aug 25, 2015, at 9:09 AM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 25 August 2015 at 06:46, Ken Thomases <ken(a)codeweavers.com> wrote:
On Aug 24, 2015, at 10:49 PM, Matt Durgavich <mattdurgavich(a)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.
If the idea is just to keep track of all the sockets you give out you want neither of those of course, but just something like a handle table or a list.
I think the intent was to make it efficient to find a given socket handle in the list. However, that may be premature optimization. Whether it's worth using a hash table for that depends on how many sockets a process is likely to open and how frequently it does an operation that needs to check the list. For example, the patch adds a check of the list in WS2_sendto(). I'm not sure why it adds that check (and only to that specific function), but you wouldn't want to slow that down too much. -Ken
On 26 August 2015 at 21:50, Ken Thomases <ken(a)codeweavers.com> wrote:
On Aug 25, 2015, at 9:09 AM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
If the idea is just to keep track of all the sockets you give out you want neither of those of course, but just something like a handle table or a list. I think the intent was to make it efficient to find a given socket handle in the list.
You'd just lookup the handle in the table. Of course it would probably be much easier to just do it in the server, since it already keeps track of socket handles.
Bruno wrote a patch a while back that tracked sockets in a linked list, and the feedback there was to use a hashing scheme similar to how files are handled since sockets, like files, can be opened and closed rapidly. Chip made this point re ws2_32 tracking sockets: "The second option I gave might be preferable, because e.g. someone could DuplicateHandle() a winsock handle into another process, and WSACleanup() might not close it when it runs. (Someone should write a test for that.)” For the server side approach, I’d need to map the fd back to a handle that can be closed. I imagine a server call is somewhat pricey, but WSACleanup shouldn’t be called often. Is there more subtly to the problem than that? Can we get a consensus here as to the right approach? I don’t want to polish a patch that is wrong-headed. Thanks, -Matt
On Aug 26, 2015, at 4:59 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 26 August 2015 at 21:50, Ken Thomases <ken(a)codeweavers.com> wrote:
On Aug 25, 2015, at 9:09 AM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
If the idea is just to keep track of all the sockets you give out you want neither of those of course, but just something like a handle table or a list. I think the intent was to make it efficient to find a given socket handle in the list.
You'd just lookup the handle in the table. Of course it would probably be much easier to just do it in the server, since it already keeps track of socket handles.
On 26 August 2015 at 23:28, Matt Durgavich <mattdurgavich(a)gmail.com> wrote:
Bruno wrote a patch a while back that tracked sockets in a linked list, and the feedback there was to use a hashing scheme similar to how files are handled since sockets, like files, can be opened and closed rapidly.
I must have missed that mail. Do you have a message ID or a link to the mailing list archive?
Chip made this point re ws2_32 tracking sockets:
"The second option I gave might be preferable, because e.g. someone could DuplicateHandle() a winsock handle into another process, and WSACleanup() might not close it when it runs. (Someone should write a test for that.)”
Tests would certainly be useful.
For the server side approach, I’d need to map the fd back to a handle that can be closed.
Well, no, you'd just enumerate all the socket handles in the process' handle table.
Can we get a consensus here as to the right approach? I don’t want to polish a patch that is wrong-headed.
I can't necessarily tell you what the right approach is, I've only spent about 10 minutes looking at the problem. But I'm fairly sure a hash table in ws2_32 isn't it.
On Thursday, August 27, 2015, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 26 August 2015 at 23:28, Matt Durgavich <mattdurgavich(a)gmail.com <javascript:;>> wrote:
Bruno wrote a patch a while back that tracked sockets in a linked list, and the feedback there was to use a hashing scheme similar to how files are handled since sockets, like files, can be opened and closed rapidly.
I must have missed that mail. Do you have a message ID or a link to the mailing list archive.
Hi, Henri. Sorry, replying from mobile phone. This was my patch at the time: http://marc.info/?l=wine-devel&m=139303064129078&w=2 Whole thread: http://marc.info/?t=138834097400003&r=1&w=2 Best wishes, Bruno
On 27 August 2015 at 13:35, Bruno Jesus <00cpxxx(a)gmail.com> wrote:
Hi, Henri. Sorry, replying from mobile phone.
This was my patch at the time: http://marc.info/?l=wine-devel&m=139303064129078&w=2
Whole thread: http://marc.info/?t=138834097400003&r=1&w=2
Yeah, I've seen that, but I don't see anyone mentioning a hash table there. If anything, I see Erich pointing to the server's handle table implementation.
On Thursday, August 27, 2015, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 27 August 2015 at 13:35, Bruno Jesus <00cpxxx(a)gmail.com <javascript:;>> wrote:
Hi, Henri. Sorry, replying from mobile phone.
This was my patch at the time: http://marc.info/?l=wine-devel&m=139303064129078&w=2
Whole thread: http://marc.info/?t=138834097400003&r=1&w=2
Yeah, I've seen that, but I don't see anyone mentioning a hash table there. If anything, I see Erich pointing to the server's handle table implementation.
I have other email asking how to list all sockets from inside the server but I didn't get any replies so at that time I thought it was not possible to do that.
On 27 August 2015 at 14:09, Bruno Jesus <00cpxxx(a)gmail.com> wrote:
I have other email asking how to list all sockets from inside the server but I didn't get any replies so at that time I thought it was not possible to do that. You'd iterate over current->process->handles->entries and compare entry->ptr->ops against sock_ops. I.e., enumerate_handles() in server/handle.c, although that seems currently unused.
Cool. I'll whip up another patch with this approach. I thought this piece of work would be straightforward ;) Thanks! Best,-Matt On Thu, Aug 27, 2015 at 8:47 AM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 27 August 2015 at 14:09, Bruno Jesus <00cpxxx(a)gmail.com> wrote:
I have other email asking how to list all sockets from inside the server but I didn't get any replies so at that time I thought it was not possible to do that. You'd iterate over current->process->handles->entries and compare entry->ptr->ops against sock_ops. I.e., enumerate_handles() in server/handle.c, although that seems currently unused.
That indeed sounds much easier than having to keep our own list inside the dll. Thanks, Henri. Bruno On Thursday, August 27, 2015, Matt Durgavich <mattdurgavich(a)gmail.com> wrote:
Cool. I'll whip up another patch with this approach. I thought this piece of work would be straightforward ;)
Thanks!
Best, -Matt
On Thu, Aug 27, 2015 at 8:47 AM, Henri Verbeet <hverbeet(a)gmail.com <javascript:_e(%7B%7D,'cvml','hverbeet(a)gmail.com');>> wrote:
On 27 August 2015 at 14:09, Bruno Jesus <00cpxxx(a)gmail.com <javascript:_e(%7B%7D,'cvml','00cpxxx(a)gmail.com');>> wrote:
I have other email asking how to list all sockets from inside the server but I didn't get any replies so at that time I thought it was not possible to do that. You'd iterate over current->process->handles->entries and compare entry->ptr->ops against sock_ops. I.e., enumerate_handles() in server/handle.c, although that seems currently unused.
Another try, this time server side. It was much simpler to do --- dlls/ws2_32/socket.c | 13 ++++++++++++- server/handle.c | 12 ++++++++++++ server/protocol.def | 3 +++ server/sock.c | 2 +- 4 files changed, 28 insertions(+), 2 deletions(-) diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c index ca82ec9..66d5b2b 100644 --- a/dlls/ws2_32/socket.c +++ b/dlls/ws2_32/socket.c @@ -1471,7 +1471,18 @@ INT WINAPI WSACleanup(void) { if (num_startup) { num_startup--; - TRACE("pending cleanups: %d\n", num_startup); + if (num_startup == 0) + { + SERVER_START_REQ(close_all_sockets) + { + wine_server_call( req ); + } + SERVER_END_REQ; + } + else + { + TRACE("pending cleanups: %d\n", num_startup); + } return 0; } SetLastError(WSANOTINITIALISED); diff --git a/server/handle.c b/server/handle.c index 5043ff7..b4b5ecb 100644 --- a/server/handle.c +++ b/server/handle.c @@ -39,6 +39,8 @@ #include "security.h" #include "request.h" +extern struct object_ops sock_ops; + struct handle_entry { struct object *ptr; /* object */ @@ -608,6 +610,16 @@ DECL_HANDLER(set_handle_info) reply->old_flags = set_handle_flags( current->process, req->handle, req->mask, req->flags ); } +DECL_HANDLER(close_all_sockets) +{ + obj_handle_t sock; + UINT index = 0; + while ( (sock = enumerate_handles(current->process, &sock_ops, &index)) ) + { + close_handle(current->process, sock); + } +} + /* duplicate a handle */ DECL_HANDLER(dup_handle) { diff --git a/server/protocol.def b/server/protocol.def index c313006..651af47 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -942,6 +942,9 @@ struct rawinput_device obj_handle_t handle; /* handle to close */ @END +/* Close all sockets for the current process */ +(a)REQ(close_all_sockets) +(a)END /* Set a handle information */ @REQ(set_handle_info) diff --git a/server/sock.c b/server/sock.c index 67d6416..2a9f444 100644 --- a/server/sock.c +++ b/server/sock.c @@ -138,7 +138,7 @@ static int sock_get_ntstatus( int err ); static int sock_get_error( int err ); static void sock_set_error(void); -static const struct object_ops sock_ops = +const struct object_ops sock_ops = { sizeof(struct sock), /* size */ sock_dump, /* dump */ -- 2.3.2 (Apple Git-55)
On Aug 27, 2015, at 9:58 AM, Bruno Jesus <00cpxxx(a)gmail.com> wrote:
That indeed sounds much easier than having to keep our own list inside the dll. Thanks, Henri.
Bruno
On Thursday, August 27, 2015, Matt Durgavich <mattdurgavich(a)gmail.com <mailto:mattdurgavich(a)gmail.com>> wrote: Cool. I'll whip up another patch with this approach. I thought this piece of work would be straightforward ;)
Thanks!
Best, -Matt
On Thu, Aug 27, 2015 at 8:47 AM, Henri Verbeet <hverbeet(a)gmail.com <javascript:_e(%7B%7D,'cvml','hverbeet(a)gmail.com');>> wrote:
On 27 August 2015 at 14:09, Bruno Jesus <00cpxxx(a)gmail.com <javascript:_e(%7B%7D,'cvml','00cpxxx(a)gmail.com');>> wrote:
I have other email asking how to list all sockets from inside the server but I didn't get any replies so at that time I thought it was not possible to do that. You'd iterate over current->process->handles->entries and compare entry->ptr->ops against sock_ops. I.e., enumerate_handles() in server/handle.c, although that seems currently unused.
On 28 August 2015 at 05:46, Matt Durgavich <mattdurgavich(a)gmail.com> wrote:
+DECL_HANDLER(close_all_sockets) You'll want to just move this to sock.c and avoid making sock_ops available outside sock.c. You may want to change the request name to something along the lines of "close_process_sockets", since it only closes sockets for the current process, not all sockets the server knows about.
+{ + obj_handle_t sock; + UINT index = 0; "unsigned int" would be more appropriate.
+ while ( (sock = enumerate_handles(current->process, &sock_ops, &index)) ) + { + close_handle(current->process, sock); + } +} + The formatting here is a bit different from the rest of the server. (I.e., spaces around function parameter lists, no spaces around control statement conditions.)
On Fri, Aug 28, 2015 at 8:30 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 28 August 2015 at 05:46, Matt Durgavich <mattdurgavich(a)gmail.com> wrote:
+DECL_HANDLER(close_all_sockets) You'll want to just move this to sock.c and avoid making sock_ops available outside sock.c. You may want to change the request name to something along the lines of "close_process_sockets", since it only closes sockets for the current process, not all sockets the server knows about.
I think a better name would be winsock_cleanup or socket_cleanup because closing the sockets may not be the only action required to proper implement WSACleanup.
+ } + else + { + TRACE("pending cleanups: %d\n", num_startup); + }
Please just drop the else and let the message be printed so the amount of cleanups and starts is always displayed in the log.
Ok, here we go. Feedback in place. dlls/ws2_32/socket.c | 18 ++++++++---------- server/handle.c | 11 ----------- server/protocol.def | 2 +- server/sock.c | 12 +++++++++++- 4 files changed, 20 insertions(+), 23 deletions(-) diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c index 66d5b2b..a4c442f 100644 --- a/dlls/ws2_32/socket.c +++ b/dlls/ws2_32/socket.c @@ -1473,16 +1473,14 @@ INT WINAPI WSACleanup(void) num_startup--; if (num_startup == 0) { - SERVER_START_REQ(close_all_sockets) - { - wine_server_call( req ); - } - SERVER_END_REQ; - } - else - { - TRACE("pending cleanups: %d\n", num_startup); - } + SERVER_START_REQ(close_process_sockets) + { + wine_server_call( req ); + } + SERVER_END_REQ; + } + + TRACE("pending cleanups: %d\n", num_startup); return 0; } SetLastError(WSANOTINITIALISED); diff --git a/server/handle.c b/server/handle.c index b4b5ecb..53bf50a 100644 --- a/server/handle.c +++ b/server/handle.c @@ -39,8 +39,6 @@ #include "security.h" #include "request.h" -extern struct object_ops sock_ops; - struct handle_entry { struct object *ptr; /* object */ @@ -610,15 +608,6 @@ DECL_HANDLER(set_handle_info) reply->old_flags = set_handle_flags( current->process, req->handle, req->mask, req->flags ); } -DECL_HANDLER(close_all_sockets) -{ - obj_handle_t sock; - UINT index = 0; - while ( (sock = enumerate_handles(current->process, &sock_ops, &index)) ) - { - close_handle(current->process, sock); - } -} /* duplicate a handle */ DECL_HANDLER(dup_handle) diff --git a/server/protocol.def b/server/protocol.def index 651af47..fb63da1 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -943,7 +943,7 @@ struct rawinput_device @END /* Close all sockets for the current process */ -(a)REQ(close_all_sockets) +(a)REQ(close_process_sockets) @END /* Set a handle information */ diff --git a/server/sock.c b/server/sock.c index 2a9f444..c077755 100644 --- a/server/sock.c +++ b/server/sock.c @@ -138,7 +138,7 @@ static int sock_get_ntstatus( int err ); static int sock_get_error( int err ); static void sock_set_error(void); -const struct object_ops sock_ops = +static const struct object_ops sock_ops = { sizeof(struct sock), /* size */ sock_dump, /* dump */ @@ -1383,3 +1383,13 @@ DECL_HANDLER(get_socket_info) release_object( &sock->obj ); } + +DECL_HANDLER(close_process_sockets) +{ + obj_handle_t sock; + unsigned int index = 0; + while ( (sock = enumerate_handles(current->process, &sock_ops, &index)) ) + { + close_handle(current->process, sock); + } +} -- 2.3.2 (Apple Git-55)
On Aug 28, 2015, at 10:37 AM, Bruno Jesus <00cpxxx(a)gmail.com> wrote:
On Fri, Aug 28, 2015 at 8:30 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 28 August 2015 at 05:46, Matt Durgavich <mattdurgavich(a)gmail.com> wrote:
+DECL_HANDLER(close_all_sockets) You'll want to just move this to sock.c and avoid making sock_ops available outside sock.c. You may want to change the request name to something along the lines of "close_process_sockets", since it only closes sockets for the current process, not all sockets the server knows about.
I think a better name would be winsock_cleanup or socket_cleanup because closing the sockets may not be the only action required to proper implement WSACleanup.
+ } + else + { + TRACE("pending cleanups: %d\n", num_startup); + }
Please just drop the else and let the message be printed so the amount of cleanups and starts is always displayed in the log.
And the renaming patch dlls/ws2_32/socket.c | 2 +- server/protocol.def | 2 +- server/sock.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c index a4c442f..b5d5ff5 100644 --- a/dlls/ws2_32/socket.c +++ b/dlls/ws2_32/socket.c @@ -1473,7 +1473,7 @@ INT WINAPI WSACleanup(void) num_startup--; if (num_startup == 0) { - SERVER_START_REQ(close_process_sockets) + SERVER_START_REQ(socket_cleanup) { wine_server_call( req ); } diff --git a/server/protocol.def b/server/protocol.def index fb63da1..2dccb9a 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -943,7 +943,7 @@ struct rawinput_device @END /* Close all sockets for the current process */ -(a)REQ(close_process_sockets) +(a)REQ(socket_cleanup) @END /* Set a handle information */ diff --git a/server/sock.c b/server/sock.c index c077755..c5c5083 100644 --- a/server/sock.c +++ b/server/sock.c @@ -1384,7 +1384,7 @@ DECL_HANDLER(get_socket_info) release_object( &sock->obj ); } -DECL_HANDLER(close_process_sockets) +DECL_HANDLER(socket_cleanup) { obj_handle_t sock; unsigned int index = 0; -- 2.3.2 (Apple Git-55)
On Aug 28, 2015, at 10:37 AM, Bruno Jesus <00cpxxx(a)gmail.com> wrote:
On Fri, Aug 28, 2015 at 8:30 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 28 August 2015 at 05:46, Matt Durgavich <mattdurgavich(a)gmail.com> wrote:
+DECL_HANDLER(close_all_sockets) You'll want to just move this to sock.c and avoid making sock_ops available outside sock.c. You may want to change the request name to something along the lines of "close_process_sockets", since it only closes sockets for the current process, not all sockets the server knows about.
I think a better name would be winsock_cleanup or socket_cleanup because closing the sockets may not be the only action required to proper implement WSACleanup.
+ } + else + { + TRACE("pending cleanups: %d\n", num_startup); + }
Please just drop the else and let the message be printed so the amount of cleanups and starts is always displayed in the log.
On Sat, Aug 29, 2015 at 6:28 AM, Matt Durgavich <mattdurgavich(a)gmail.com> wrote:
And the renaming patch
Please attach a single patch that is based against the original file, it's unnecessary harder to review an incremental patch.
All suggestions rolled in. Removing todos in tests will be a separate patch
From d62ed3e109f6d6a862f62afb1242ca15e199fd98 Mon Sep 17 00:00:00 2001 From: Matt <matt(a)wymzee.com> Date: Thu, 27 Aug 2015 23:43:59 -0400 Subject: [PATCH] WS2_32: WSACleanup implementation, server-side.
--- dlls/ws2_32/socket.c | 9 +++++++++ server/protocol.def | 3 +++ server/sock.c | 10 ++++++++++ 3 files changed, 22 insertions(+) diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c index ca82ec9..b5d5ff5 100644 --- a/dlls/ws2_32/socket.c +++ b/dlls/ws2_32/socket.c @@ -1471,6 +1471,15 @@ INT WINAPI WSACleanup(void) { if (num_startup) { num_startup--; + if (num_startup == 0) + { + SERVER_START_REQ(socket_cleanup) + { + wine_server_call( req ); + } + SERVER_END_REQ; + } + TRACE("pending cleanups: %d\n", num_startup); return 0; } diff --git a/server/protocol.def b/server/protocol.def index c313006..2dccb9a 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -942,6 +942,9 @@ struct rawinput_device obj_handle_t handle; /* handle to close */ @END +/* Close all sockets for the current process */ +(a)REQ(socket_cleanup) +(a)END /* Set a handle information */ @REQ(set_handle_info) diff --git a/server/sock.c b/server/sock.c index 67d6416..c5c5083 100644 --- a/server/sock.c +++ b/server/sock.c @@ -1383,3 +1383,13 @@ DECL_HANDLER(get_socket_info) release_object( &sock->obj ); } + +DECL_HANDLER(socket_cleanup) +{ + obj_handle_t sock; + unsigned int index = 0; + while ( (sock = enumerate_handles(current->process, &sock_ops, &index)) ) + { + close_handle(current->process, sock); + } +} -- 2.3.2 (Apple Git-55)
On Aug 29, 2015, at 10:43 AM, Bruno Jesus <00cpxxx(a)gmail.com> wrote:
On Sat, Aug 29, 2015 at 6:28 AM, Matt Durgavich <mattdurgavich(a)gmail.com> wrote:
And the renaming patch
Please attach a single patch that is based against the original file, it's unnecessary harder to review an incremental patch.
On Sun, Aug 30, 2015 at 2:13 AM, Matt Durgavich <mattdurgavich(a)gmail.com> wrote:
All suggestions rolled in. Removing todos in tests will be a separate patch
You can't do that, because the changes you are making will break the tests (tests will fail since problems were fixed and todo_wine need to be removed). Just merge the todo removal and IMO the patch is fine. Remember to run make test inside the ws2_32/tests folder to check if it runs fine. It amazes me that such a simple solution could be found for this bug, thanks for working on it and thanks Henri for the tip.
Cool. Thanks for all the feedback. I’m happy to be contributing, and I got a crash course in server and dll development. :)
On Aug 29, 2015, at 2:19 PM, Bruno Jesus <00cpxxx(a)gmail.com> wrote:
On Sun, Aug 30, 2015 at 2:13 AM, Matt Durgavich <mattdurgavich(a)gmail.com> wrote:
All suggestions rolled in. Removing todos in tests will be a separate patch
You can't do that, because the changes you are making will break the tests (tests will fail since problems were fixed and todo_wine need to be removed). Just merge the todo removal and IMO the patch is fine. Remember to run make test inside the ws2_32/tests folder to check if it runs fine.
It amazes me that such a simple solution could be found for this bug, thanks for working on it and thanks Henri for the tip.
Responses inline. Best,-Matt On Tuesday, Aug 25, 2015 at 12:46 AM, Ken Thomases <ken(a)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(a)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.
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(a)gmail.com> wrote:
Responses inline.
Best, -Matt
On Tuesday, Aug 25, 2015 at 12:46 AM, Ken Thomases <ken(a)codeweavers.com <mailto:ken(a)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(a)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.
On Aug 25, 2015, at 2:18 PM, Matt Durgavich <mattdurgavich(a)gmail.com> wrote:
@@ -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;; + } +
I raised this in my reply to Henri, but: why was this added? And if it's sensible to have it here, why not in any of the other functions?
+static void cache_update(SOCKET new_entry, SOCKET old_entry)
Still calling it a "cache"?
+{ + 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)
I think it would be better to do: if (*slot == old_entry && InterlockedCompareExchange(slot, new_entry, old_entry) == old_entry) While InterlockedCompareExchange() is faster than locking, it's still not cheap. Doing it on every element without the prior belief that that element matches seems wasteful.
+ break; + if (depth == BUCKET_DEPTH) + { + ++tries; + depth = 0; + } + + } while (tries < 3); + if (tries == 3) + { + ERR("Socket hash table bucket overflow. Resize buckets"); + } +}
I don't see any reason to try multiple times. The chances of a slot changing from one pass to the next is miniscule. If you're searching for a free slot (old_entry is 0) and you don't find one on the first pass, then you should still resize the buckets, even if you would hypothetically find one on a second or third pass. If old_entry is not 0, then it's really unlikely that another thread would add that specific socket to the table. How could that happen that wouldn't be a bug?
+ +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;
This is two passes when one should do. More importantly, though, it seems race-prone. If two threads are both closing the same socket, they could each first determine that the socket is in the table and thus return TRUE, when only one should return TRUE. You could make cache_udpate() return a success/failure value and simply make this a wrapper around cache_update(0, s). -Ken
participants (5)
-
Bruno Jesus -
Charles Davis -
Henri Verbeet -
Ken Thomases -
Matt Durgavich