ws2_32: WSACleanup cleans up open sockets (OSX only)

[Matt Durgavich]

Responses inline. 




Best,-Matt




On Tuesday, Aug 25, 2015 at 12:46 AM, Ken Thomases <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/4231af66/attachment.html>