-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Workaround for recv() buffer flushed when send() returns error on closed connection #668
Conversation
55feaba
to
debbdb9
Compare
First, there needs to be proper precautions against overflowing that fixed-size buffer, as you don't ever clear the variable that adds data to the buffer. Do we really need to read unconditionally before every send() ? I suspect that may hamper transfer speeds, especially when doing many parallel transfers. Also, it is unfortunate to add ~16K of data to every easy handle for this. Then, I would like the '#ifdef' to rather be for a name that describes what its for so that we can build and run the code on non-windows platforms too to make sure it works and doesn't ruin anything. When talking non-plaintext protocols, like TLS, this work-around will probably not work anyway since the TLS library can very well both read and write data on its own. I would still like this Window TCP stack bug acknowledged by an official page/forum/person so that we know when to switch it off and also if it could just be something we do that triggers it. |
That would then be
Absolutely. I suspect Winsock is working-as-designed. Seems related to how libcurl is shoehorning |
I was even thinking
I don't think so, as @Karlson2k speaks of problems when doing HTTP POST and the telnet "shoehorning" is only for telnet. |
I did not assign the value because I suspect that structure is nullified somewhere. Anyway I used this mostly like a POC, real code polishing is not done yet.
Not recv() - select() with single socket before every send(). This is transformed internally to NtDeviceIoControlFile() and NtWaitForSingleObject() (or may be only NtDeviceIoControlFile() - didn't try to go deep with zero timeout). With nonblocking socket we can simply try to recv(), but I'm not sure that sockets are always non-blocking in libcurl.
Don't think so, but only real testing can prove any difference. If difference will not be neglectable, than we can add some Win32 runtime option to disable this workaround in favor for speed.
We can replace it with curl buffers, but need to carefully checking that this are freed.
Code call libcurl wrapper for recv() so this should work with TLS as well - this just gives TLS lib chance to read early.
So do I, but there is not official way to manually report WinSock bugs to Microsoft. Will try to find suitable official forum. If you know any - let me know, please.
Cygwin use wrapper over WinSock so libcurl on Cygwin doesn't use WinSock (directly), but workaround is still required. |
If libcurl sends HTTP POST in one |
Right, but that's because of other reasons... |
May be, or may be not. We didn't know. 😄 |
That won't suffice. Recall that we could be uploading a 4GB file and the response could be a 4GB response body that starts streaming down while the upload is going on. The protocol allows this. |
In addition to that, both easy handles and connections can be re-used for subsequent transfers. |
@bagder ... and the telnet "shoehorning" is only for telnet. Ok, but do you agree (with no bias) that liburl/Win32 has some "shoehorning" or protocol tweaks? If yes, why should Telnet be handled specially? Hence looking at the big-picture of how libcurl vs other programs do socket I/O, something can be learned as to why libcurl needs such tweaks in the first place. For example, it would be interesting to know exactly how a C++ program using Qt5 or Boost.Asio would behave in such a case. This Winsock "bug" would have caused hysteria. Here is the design overview of Boost.Asio: What the inner detail of |
It really shouldn't, it just still is and nobody has fixed it. In the telnet case I think mostly because virtually nobody ever use telnet so it doesn't hurt much. I know for me that's why I don't bother much with cleaning that up.
I can't but to think the same, which is also partly why I'd like it acknowledged/dismissed by some sort of official party before we proceed and sprinkle the code with weirdo code. |
Not the wording I'd chose, but LOL 😄 . |
Code already handles all above. In other words: |
Ah yes sorry I forgot that when I then wrote the comment, Is reading only the first 16K really what you want? If the bug is that the last received data (before a send) is lost, then shouldn't the code make sure that the last data is read out from the kernel's socket buffer? But I would like to take a step back first. Can we produce a stand-alone test case to reproduce this bug with so that we can try it out on different windows versions and see where we have the problems and possibly get to understand it even further? I would like our work-around code to be as narrow and precise as possible. |
Yes, only last small amount of data, usually a not more than 500 bytes.
It's already present. Not curl-based, but very simplified and cross-platform, I published it in issue #657 description: I was trying to add libcurl test, but I'm too deep on fixing curl testsuite errors on W32. |
If this bug is reproducible on different versions of Windows, this should probably be reported to Microsoft. They might come up with a KB or Fixit for this in that case. |
debbdb9
to
fda70e2
Compare
@bagder Patch updated with little bit more elegant version. Now passed test 1517 from PR #720 and libmicrohttpd testsuite.
Could you give me some hint, which place of code is best to add buffer reset for new connection? |
if((conn->handler->protocol&PROTO_FAMILY_HTTP) != 0 && | ||
conn->recv[num] == Curl_recv_plain) { | ||
const int readymask = Curl_socket_check(sockfd, CURL_SOCKET_BAD, | ||
CURL_SOCKET_BAD, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if Curl_send_plain()
is called twice (with incoming data present) before Curl_recv_plain()
is called? Doesn't it need to append the new data to the temporary buffer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already handled by
ssize_t recvedbytes = sread(sockfd, st->postponed_buffer +
st->recv_postponed_size, bytestorecv);
Buffer is allocated once, assuming that it will be enough.
May be need better names, or someone can be confused with postponed_buffer_size
and recv_postponed_size
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, when st->recv_postponed_size
grows to st->postponed_buffer_size
it will ask to read 0 bytes. But is there actually anything that says that is a safe assumption and shouldn't the code at least avoid the case when there's no space left to read data into?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const size_t bytestorecv = st->postponed_buffer_size -
st->recv_postponed_size;
if(bytestorecv) {
ssize_t recvedbytes = sread(sockfd, st->postponed_buffer +
st->recv_postponed_size, bytestorecv);
If st->recv_postponed_size
grows to st->postponed_buffer_size
then bytestorecv
is zero is recv()
is skipped.
I'd like to keep this patch as simple as possible and less intrusive.
Don't think that we need to handle any really huge buffers here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just thought that if there's no space left to read data, there's no point in doing the Curl_socket_check()
call to begin with... But that's not a biggie.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be rare situation.
However you're right, Curl_socket_check()
is not expensive call, but it produce a system call, so better to avoid it if possible.
The title "Workaround for WinSock bug with destroyed recv() buffer if TCP connection was closed remotely". But how is this closure detected? Isn't it detected by send() failing and we bail out on that? |
I could only imagine how WinSock internals work. But I suspect that WinSock detect remote disconnection and store it in some socket-specific structure. If |
I want to clarify the title of this issue so I'm asking how libcurl detects the remote close in this case. I think it is detected by send() returning a failure. Isn't that what happens? |
libcurl doesn't detect remote close, libcurl just saves incoming data before sending. |
Sure it detects remote close, by the corresponding call returning 0. |
WinSock bug is triggered by calling |
*code = CURLE_OK; | ||
return nread; | ||
} | ||
#endif /* USE_WINSOCK_WORKAROUNDS */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here as well.
BTW as the added select() will unconditionally do a system call and then you might do another with the added recv(), isn't it faster to just blindly try a recv() without any select() first ? |
@bagder Are sockets always non-blocking? |
066565f
to
02eee28
Compare
@bagder Updated. |
bde568d
to
1ce0feb
Compare
@bagder Updated again, syntax fixed. |
1ce0feb
to
67acc5b
Compare
@bagder And if recv() is blindly called each time, then extra buffer will be allocated for each socket, which is system call too. |
a775c76
to
12593f9
Compare
response when POST request is used with slow read callback function. Test should check for presence of WinSock bug curlgh-657 and ability to workaround it curlgh-668. Bug: curlgh-657 Workaround: curlgh-668
@bagder Updated with proper commit description. |
Any chances to merge it somewhen soon? |
It doesn't merge cleanly right now, can you rebase it please? |
WinSock destroys recv() buffer if send() is failed. As result - server response may be lost if server sent it while curl is still sending request. This behavior noticeable on HTTP server short replies if libcurl use several send() for request (usually for POST request). To workaround this problem, libcurl use recv() before every send() and keeps received data in intermediate buffer for further processing. Bug: curlgh-657
12593f9
to
d98e552
Compare
Was this ever reported to Microsoft and also this workaround seems to be only for plaintext? Apologies if these questions were already answered, it's been a while and a cursory read of this I can't find the answers. |
I'm not aware of any report, but I'm also not entirely sure this is against any spec or documentation. I haven't found anything that clearly says this isn't permitted behavior. So the problem is then possibly "just" a libcurl problem when we |
Should fix issue #657.
Tested locally - works good.
Not sure that I followed correctly coding style and didn't break anything. Anyway, I'll improve it if needed.
_WIN32
in ifdefs can be replaced with some winsock marker.If code itself is OK, I'd like to add comments before merging.
May be code for early recv() must be moved to one function instead of duplicating in two places.
The code is simple and don't receive more data than fits into the single buffer, but that shouldn't be a problem as those early responses are usually a short error messages with sizes < 200 bytes.