Hi Daniel,
I have some concerns wrt your WSAAccept implementation you sent to wine-patches on Sunday. I have thought a bit about this function (did you notice my preliminary implementation on Nov. 7 (subject "[EXPERIMENTAL PATCH]: Winsock2" ?).
The problem I see is related to the treatment of the peer when the user-supplied condition function returns CF_REJECT or CF_DEFER.
Your implementation calls WS_Accept _before_ calling the condition function. That is, the peer will think its connection request was accepted, and possibly start to send or poll for data. Then if the Condition function returns CF_REJECT, the connection is closed.
Even worse, for CF_DEFER, the peer receives nothing, the new socket cs returned by WS_Accept remains open but is forgotten after WSAAccept() returns (correct me if I'm wrong). So the peer will think it's connected, but no future WSAAccept or WS_Accept call will ever return a socket connected to it, and it'll be sending data into the void.
I think you must at least closesocket(cs) for CS_DEFER as well to avoid this situation. Alternatively (that's the way I did it) the new socket can be stored in the wineserver to be returned by future WSAAccept calls.
The Winsock2 specs say that "[WSAAccept] extracts the first connection on the queue of pending connections" (after a CF_DEFER, that should be the previously deferred connection, until it's either accepted or rejected). Moreover, it says that "[In the CF_DEFER case] no action about this connection request should be taken by the service provider". Taken seriously, this means that it's wrong to call accept() on this request before a positive decision was made by the Condition function.
Summarizing, the peer should see:
CF_ACCEPT: his request is accepted (this case is OK). CF_REJECT: his request is rejected (not ok, he receives an accept followed by a close). CF_DEFER: nothing (not OK, he receives an accept followed by ???)
I have no idea how this affects behaviour of "real" applications.
The essential problem is that there is no way (well, I have found none) to obtain the peer's socket address on Linux without accept()ing first. I guess that is what made you call accept() before the condition routine. Perhaps someone can come up with a method to read the socket address of a waiting connection request before it's accepted, but my guess is this won't work without a kernel hack.
At the very least, though, I consider it necessary to include a FIXME that tells users that this function may behave different from what the specs say.
Please don't get me wrong, it's great to see other people work on Winsock, and your code is clean. I just thought I need to express these concerns.
Regards, Martin
Martin Wilck wrote:
Hi Daniel,
I have some concerns wrt your WSAAccept implementation you sent to wine-patches on Sunday. I have thought a bit about this function (did you notice my preliminary implementation on Nov. 7 (subject "[EXPERIMENTAL PATCH]: Winsock2" ?).
I found it after I had submitted mine ..
The problem I see is related to the treatment of the peer when the user-supplied condition function returns CF_REJECT or CF_DEFER.
Your implementation calls WS_Accept _before_ calling the condition function. That is, the peer will think its connection request was accepted, and possibly start to send or poll for data. Then if the Condition function returns CF_REJECT, the connection is closed.
When I was writing this function I didn't find a good way to get information on the connections before accepting() them ..
Even worse, for CF_DEFER, the peer receives nothing, the new socket cs returned by WS_Accept remains open but is forgotten after WSAAccept() returns (correct me if I'm wrong). So the peer will think it's connected, but no future WSAAccept or WS_Accept call will ever return a socket connected to it, and it'll be sending data into the void.
I think you must at least closesocket(cs) for CS_DEFER as well to avoid this situation. Alternatively (that's the way I did it) the new socket can be stored in the wineserver to be returned by future WSAAccept calls.
The Winsock2 specs say that "[WSAAccept] extracts the first connection on the queue of pending connections" (after a CF_DEFER, that should be the previously deferred connection, until it's either accepted or rejected). Moreover, it says that "[In the CF_DEFER case] no action about this connection request should be taken by the service provider". Taken seriously, this means that it's wrong to call accept() on this request before a positive decision was made by the Condition function
Point taken .. I realized this when I was looking at your patch a few days ago ..
Summarizing, the peer should see:
CF_ACCEPT: his request is accepted (this case is OK). CF_REJECT: his request is rejected (not ok, he receives an accept followed by a close). CF_DEFER: nothing (not OK, he receives an accept followed by ???)
I have no idea how this affects behaviour of "real" applications.
The essential problem is that there is no way (well, I have found none) to obtain the peer's socket address on Linux without accept()ing first. I guess that is what made you call accept() before the condition routine. Perhaps someone can come up with a method to read the socket address of a waiting connection request before it's accepted, but my guess is this won't work without a kernel hack.
Right.
At the very least, though, I consider it necessary to include a FIXME that tells users that this function may behave different from what the specs say.
Please don't get me wrong, it's great to see other people work on Winsock, and your code is clean. I just thought I need to express these concerns.
No problem .. Although, Something is better than nothing .. I have an application that uses this function, and I wanted that application to work .. Even though my implementation is not complete, since it's now in the main wine source it can mature .. By withholding your implementation your not doing anyone any favors ..
Daniel Walker
Dear Daniel,
I seem to have problems sending mail directly to your address ...
No problem .. Although, Something is better than nothing .. I have an application that uses this function, and I wanted that application to work .. Even though my implementation is not complete, since it's now in the main wine source it can mature .. By withholding your implementation your not doing anyone any favors ..
Yeah, that was my very first wine patch ever, and I didn't understand the rules of wine-devel & wine-patches by that time. Afterwards I forgot about it, concentrating on overlapped IO, and was reminded of it by your patch.
It is good to see that we seem to be in basic agreement, though. After all, it will probably happen very rarely that an app rejects or even defers clients.
Btw: will you be working on more Winsock stuff in the near future? If yes, I guess we should try to coordinate our efforts somehow.
Cheers, Martin
Martin Wilck wrote:
Dear Daniel,
I seem to have problems sending mail directly to your address ...
You have a "g" on the end instead of a "k" . diwalker@earthlink.net (not earthling) .
No problem .. Although, Something is better than nothing .. I have anapplication that uses this function, and I wanted that application to work .. Even though my implementation is not complete, since it's now in the main wine source it can mature .. By withholding your implementation your not doing anyone any favors ..
Yeah, that was my very first wine patch ever, and I didn't understand the rules of wine-devel & wine-patches by that time. Afterwards I forgot about it, concentrating on overlapped IO, and was reminded of it by your patch.
I hope you submit it .. I think it was more than just WSAAccept() ..
It is good to see that we seem to be in basic agreement, though. After all, it will probably happen very rarely that an app rejects or even defers clients.
I would guess that rejection might be common, but I can't see many apps taking advantage of defer ..
Btw: will you be working on more Winsock stuff in the near future? If yes, I guess we should try to coordinate our efforts somehow.
I don't have anything planned, right now ..
Daniel Walker
-
Daniel Walker -
Martin Wilck