RFC: Revised patchset to fix Bug 7929 (C&C 3 network does not work)
mike.kaplinskiy at gmail.com
Sun Sep 19 14:13:57 CDT 2010
On Sun, Sep 19, 2010 at 1:33 PM, Erich Hoover <ehoover at mines.edu> wrote:
> 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).
Thanks that helps a bit. You could still do a few more of those
cleanups. Also please don't make a function like WS2_shim_poll; either
make it poll-like or call it WS2_shim_select.
>> 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.
That's what I mean - if you need to do filtering for ReadFile - do it
in ReadFile (conditionally, there is a way to check if the fd is a
socket). Besides the little scheme in the server would only fix async
ReadFile requests. If you just called ReadFile with some data pending
you would still get it even if it wasn't to the right interface.
>> ... 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()...
Yes, except do_block would return on any packet received, WS2_recv
would return 0 and we would return 0 saying "connection closed" which
may not even make sense. We don't really have any tests for UDP which
is why you're not seeing any test failures. You fixed it in this set
though, but you can still have funky stuff happening (returning 0 from
recv because all the packets were filtered out). You really need to
return EWOULDBLOCK from WS2_recv for it all to fix itself.
>> 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".
It's not. Only async requests are. FYI NtReadFile just calls read().
>> 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
Applications that don't sign up for EWOULDBLOCK (i.e. blocking reads)
wouldn't get it since we handle blocking ourselves. WS2_async_recv
handles EWOULDBLOCK to return STATUS_PENDING just fine.
>> 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
We emulate blocking (see _is_blocking & do_block) ourselves, the
actual *nix sockets always have O_NONBLOCK set.
>> 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.
Cool, I didn't realize that. I guess with the way MS designed it
IP_PKTINFO would still work correctly.
>> 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.
This will probably have to be Alexandre's call. I was referring to his
first comment mostly.
>> 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.
That just fixes the problem in ws2_32. We still can't use poll on
sockets anywhere else (notably the server). As a consequence FD_READ
socket events would now fire for every packet. To fix that you filter
out POLLIN when necessary (which is ok), just your reasoning for doing
it is a little wrong. WSARecv overlapped requests end up on that queue
>> 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.
It's really unlikely that you wouldn't see it. If you print out fd's
on the server side and the wine-userspace side they should be
different. We are effectively dup'ing them to get them to wine
> 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
More information about the wine-devel