RFC: Revised patchset to fix Bug 7929 (C&C 3 network does not work)

[Erich Hoover]

On Sat, Sep 18, 2010 at 9:59 PM, Mike Kaplinskiy
<mike.kaplinskiy at gmail.com> wrote:
> There's enough #ifdef's to make this mostly unreadable.
Sorry about this, I was concerned that pushing these out so that
changes do not appear directly in the relevant code would make things
too obfuscated.  Please look at the revised patchset (at bottom).

> Pick a place to do the filtering and stick with it.
>You do some filtering in the server, some filtering in wine-userspace ...
The filtering in the server is done to handle throwing packets away
from async polls and calls that come through ReadFile.

> ... and some calls aren't filtered at all (see blocking recv's).
The blocking WS_recv() is forwarded to WS2_recvfrom(), which forwards
to the "async" WS2_recv()...

> I would suggest forwarding ReadFile/WriteFile to WSARecv/WSASend for sockets and then taking care of it in ws2_32 (we might end up needing this soon anyway for other annoying reasons).
I'll have to look into this, I am not familiar enough with how Wine
handles these operations to make such a change at this time.  The
ReadFile portion is at least handled through the server-side
filtering, though a WriteFile request would go out on all interfaces
I'd say that's "not too terrible".

> Even if your async reads are woken, you should be lenient enough in WS2_recv to return EWOULDBLOCK if no packets matching the interface are available. Since you don't do this now blocking recv's are broken.
This scenario is part of why some of the filtering occurs on the
server side, in all honestly, fixing this issue took a long time to
work out.  It was previously pointed out that returning EWOULDBLOCK in
these scenarios would break "broken" applications that do not handle
EWOULDBLOCK.

> Also, FYI our *nix sockets are always non-blocking, you don't need MSG_DONTWAIT.
This is contrary to both my experience and the manual pages for recv()
and fcntl()... *nix sockets are non-blocking when O_NONBLOCK is set,
which is not necessarily the case as Wine handles both types of
sockets.

> So what if an application likes to bind to an interface AND use IP_PKTINFO?
According to MSDN, while getting the IP_PKTINFO flag is supported,
setting it is not.

> Alexandre's comments from last year still apply.
Unless you're talking about the problem where descriptors are not
invariant enough then I don't see how, these changes should make the
peek/read combo safe across threads and processes.

> For example, now we wouldn't be able to use poll anywhere reliably on sockets.
This case should be handled in one of the patches, the revised set
(see bottom) should make this more obvious.  It looks like I missed
locking around some polls, this issue should be corrected in the
revised set.

> If you really want to get this to work consider writing something to LD_PRELOAD that emulates most of the network syscalls (+ read) with windows-style bind'ing correctly. I'd argue this may be easier and faster than putting the fix it into wine. This may be more user-friendly than having people compile wine themselves.
I'm not interested in taking the easy way out here, I would much
rather put the appropriate effort into having Wine handle this
scenario than making a one-off fix.

On Sat, Sep 18, 2010 at 10:22 PM, Mike Kaplinskiy
<mike.kaplinskiy at gmail.com> wrote:
> Ah sorry, forgot to mention one more thing - fd's are different on the server side and the client side and between processes. So you're locking different semaphores everywhere effectively not locking anything.
I did not see this in testing, but I understand that it's possible
that different systems do not behave the same way.  In the revised set
(see bottom) I have implemented Damjan's suggestion, which works just
as effectively in the testing I've done.

Thank you both for your comments, I hope you find the revised patches
below to be more palatable.

[1/3] http://www.compholio.com/wine-kane/patches/2010-09-19/0001-ws2_32-tests-Add-UDP-broadcast-tests.patch
[2/3] http://www.compholio.com/wine-kane/patches/2010-09-19/0002-server-Add-mechanism-for-storing-an-interface-ID-wit.patch
[3/3] http://www.compholio.com/wine-kane/patches/2010-09-19/0003-ws2_32-Patch-to-selectively-bind-to-interfaces-while.patch

Erich Hoover
ehoover at mines.edu