Skip to content
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

HTTP response could not be read if using POST on Windows #657

Closed
Karlson2k opened this issue Feb 15, 2016 · 7 comments
Closed

HTTP response could not be read if using POST on Windows #657

Karlson2k opened this issue Feb 15, 2016 · 7 comments

Comments

@Karlson2k
Copy link
Contributor

This is actually a WinSock bug, but workaround can be implemented.
I discovered it while debugging GNU libmicrohttpd test suite.
Problem: WinSock destroy received data buffer if it detected TCP closure by remote side.

In practice:

  1. libcurl send HTTP request header
  2. HTTP server receive HTTP request header, replied with some HTTP error code and closed connection
  3. libcurl continue to send HTTP request (request body)
  4. WinSock on libcurl side detect rejection of data by remote side, set socket state to "closed by remote host"
  5. libcurl call recv(), but WinSock returns error WSAECONNABORTED or WSAECONNRESET. Received data is destroyed by WinSock. libcurl returns network error instead of real HTTP code.

To illustrate it, I've created small test suite:
https://github.com/Karlson2k/check_system_socket_recv_last

This bug is the reason why function testMultithreadedPostCancelPart in https://github.com/Karlson2k/libmicrohttpd/blob/master/src/testcurl/test_post.c#L538 failed when called with FLAG_SLOW_READ on Windows.

To workaround this Windows bug, libcurl must call recv() ASAP if incoming data is detected in CURLM_STATE_DOING, CURLM_STATE_DO_MORE states. Not sure that it can be easly implemented without breaking something.

@bagder
Copy link
Member

bagder commented Feb 16, 2016

To workaround this Windows bug, libcurl must call recv() ASAP if incoming data is detected in ...

Thanks!

I don't get why just those two states are important. Shouldn't we then read data as soon as possible in any state if we want to get the data sent before the server closes the connection (sending a FIN or RST I guess?)? But then: aren't we already?

Can you link to more info about this windows bug?

Why aren't we seeing more problems with this bug if it really is this blatantly stupid?

@Karlson2k
Copy link
Contributor Author

I don't get why just those two states are important. Shouldn't we then read data as soon as possible in any state if we want to get the data sent before the server closes the connection (sending a FIN or RST I guess?)?

Actually in Windows case you better to always check for incoming data when trying to send something. But practically, in case of HTTP(S) you must check for incoming data when sending request.

But then: aren't we already?

In case of HTTP POST and PUT, send() can be called multiple time before any single recv() is attempted.

Can you link to more info about this windows bug?

Try my testsuite from the Issue description. It's multiplatfrom and show clearly situations where this bug is not arise and where it's in action.
Similar issue is described here: http://www.chilkatsoft.com/p/p_299.asp

Why aren't we seeing more problems with this bug if it really is this blatantly stupid?

Combination of several conditions is required:

  1. Data must be received by WinSock, but not read by recv()
  2. Connection must be closed by remote side.
  3. Some data must be attempted to send to remote after closure of connection.
  4. recv() must be called only after some delay.

Seems than final delay is only required for local loopback, when using real networks only 1-3 is enough to trigger the bug.
Try my testsuite with server_part on any remote host (virtual machine is fine).
So it's harder to trigger this bug with local testing than with real networks.

libcurl usually don't attempt to send data when expecting some incoming data from remote.
Early response from HTTP server is one of exceptions.

@jay
Copy link
Member

jay commented Feb 18, 2016

I think you should report this to Microsoft and see what they have to say.

@Karlson2k
Copy link
Contributor Author

I think that they reply similar to WSAPoll() reported problem. Anyway, no chance that problem will be fixed in Vista/7/8.

@jay
Copy link
Member

jay commented Mar 19, 2016

I think that they reply similar to WSAPoll() reported problem. Anyway, no chance that problem will be fixed in Vista/7/8.

Do you mean you think they will reply that way or you think they did reply that way? Where is the reply, who did you contact and how do you know there's no chance?

I think the delay is key here. In Windows 7 x64 (SP1 Enterprise) I changed your server example to shutdown via SD_SEND instead of SD_BOTH and there's no error. If I put SD_RECEIVE after that it's fine.

  shutdown(s, SD_SEND);
  shutdown(s, SD_RECEIVE);

As long as server send is shutdown before server receive the client will finish without any errors. The doc says:

If the how parameter is SD_RECEIVE, subsequent calls to the recv function on 
the socket will be disallowed. This has no effect on the lower protocol layers. 
For TCP sockets, if there is still data queued on the socket waiting to be 
received, or data arrives subsequently, the connection is reset, since the data 
cannot be delivered to the user. For UDP sockets, incoming datagrams are 
accepted and queued. In no case will an ICMP error packet be generated. 

If the how parameter is SD_SEND, subsequent calls to the send function are 
disallowed. For TCP sockets, a FIN will be sent after all data is sent and 
acknowledged by the receiver. 

Setting how to SD_BOTH disables both sends and receives as described above. 

I think if you want to pursue this further you will first need to monitor in Wireshark or something to figure out exactly what's happening. Also there's a separate doc for graceful shutdown you may want to read.

@Karlson2k
Copy link
Contributor Author

Just checked - only error in last test is gone, not error in test before last one.
Updated example to count total number of errors instead of using only last one.
From MSDN Graceful Shutdown:

Some amount of confusion arises, however, from the fact that the closesocket function implicitly
causes a shutdown sequence to occur if it has not already happened. In fact, it has become
a rather common programming practice to rely on this feature and to use closesocket to both
initiate the shutdown sequence and deallocate the socket handle.

However, it doesn't change the fact that WinSock destroy received data if send() function is used. If you don't call send(), WinSock will hold received data until if will be read by recv() even if connection is closed. But as soon as you call send() for same socket - WinSock will not return any data for subsequent recv().

This is very common for Web servers: in case of some internal error or other inability to process request - reply with short error description and close connection. Web server can't wait for each client to read response - it will be too costly.

The problem is only on WinSock receiver side. If you run server_part on Windows and client_part on any other OS (tested Linuxes, FreeBSD, OS X, Solaris) then client will read all received data in all cases.

@Karlson2k
Copy link
Contributor Author

By default WinSock use "graceful shutdown", from MSDN closesocket():

If the l_onoff member of the LINGER structure is zero on a stream socket, the closesocket
call will return immediately and does not receive WSAEWOULDBLOCK whether the socket is blocking
or nonblocking. However, any data queued for transmission will be sent, if possible, before
the underlying socket is closed. This is also called a graceful disconnect or close. In this
case, the Windows Sockets provider cannot release the socket and other resources for an
arbitrary period, thus affecting applications that expect to use all available sockets. This
is the default behavior for a socket.

Karlson2k added a commit to Karlson2k/curl that referenced this issue Apr 7, 2016
WinSock destroys recv() buffer if sendf() 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 sendf() and
keeps received data in intermediate buffer for further processing.

Bug: curlgh-657
Karlson2k added a commit to Karlson2k/curl that referenced this issue Apr 7, 2016
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
Karlson2k added a commit to Karlson2k/curl that referenced this issue Apr 7, 2016
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
Karlson2k added a commit to Karlson2k/curl that referenced this issue Apr 19, 2016
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
@bagder bagder closed this as completed in 72d5e14 Apr 20, 2016
bagder pushed a commit that referenced this issue Apr 20, 2016
... for checking ability to receive full HTTP response when POST request
is used with slow read callback function.

This test checks for bug #657 and verifies the work-around from
72d5e14.

Closes #720
@lock lock bot locked as resolved and limited conversation to collaborators May 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

3 participants