Re: ws2_32: WSACleanup cleans up open sockets (OSX only)

[Ken Thomases]

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