fix catching timed-out socket#599
Conversation
due to some ancient piece of code that's supposed to fix a bug in 1990's internet explorer, sockets were switched between blocking and non-blocking mode, making it hard to differentiate when socket timeouts kicked in. with the IE bug workaround removed, sockets are now always in blocking mode so we no longer need to catch EAGAIN/EWOULDBLOCK and treat them specially, they now always are treated as an error (whenever they are returned, the timeout kicked in). this should fix once and for all the cases where tinyproxy would not respect the user-provided socket timeouts, potentially causing endless loops.
|
Agreed, this is the proper fix. Treating EAGAIN/EWOULDBLOCK as "retry" in This also fixes the same class of issue inside the main relay loop (the |
|
thanks for the analysis. did you also happen to test the PR in the scenario that lead to you noticing the endless loop ? |
|
It takes some time before I am able to trigger the endless loop. Besides, I have a niche use, so I am writing my own proxy at https://github.com/renaudallard/thinproxy |
interesting. i pondered doing something similar too, in the style of microsocks because the handshake for the CONNECT method is more efficient than SOCKS5. however, even with poll(), using only one thread will soon become a bottleneck if there are any concurrent file transfers.
so just to be clear, did you actually experience any endless loops using tinyproxy when you filed #598 or did you just read the code and figure that could happen? in either case if you already have some sort of test client and server that deliberately make connections hang i'd be grateful so i dont have to write them myself. python or perl would probably be the best candidates. |
|
I am runing multiple servers in a round-robin fashion, and I noticed a hang. But it was only clear when 50%+1 servers failed and it can take some time. |
|
were these servers under your control? i figure once a thread goes into the endless loop, write/read syscalls would return immediately, causing high cpu usage. i'm asking because i'm running tinyproxy 1.11+ since years for all my connections (with uptime into several months) and never noticed neither irresponsive instances nor high cpu usage. |
|
It's running on OpenBSD and it seems connections never end properly. That's with tinyproxy not running anymore for 30+ minutes.
|
|
I submitted this for the FIN_WAIT_2 issue: #600 |
There was a problem hiding this comment.
Pull request overview
This PR removes legacy socket mode switching (originally added for an old Internet Explorer workaround) so sockets remain blocking, and adjusts buffer I/O to stop treating EAGAIN/EWOULDBLOCK as a non-error—aiming to ensure configured socket timeouts are respected and to prevent timeout-driven infinite loops.
Changes:
- Removed
socket_nonblocking()/socket_blocking()helpers and the IE POST CRLF “peek” workaround. - Simplified request entity pulling by removing the
iehackparameter and related logic. - Changed
read_buffer()/write_buffer()to no longer swallowEAGAIN/EWOULDBLOCKas “no progress”.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/sock.c |
Removes nonblocking/blocking toggling helpers to keep sockets consistently blocking. |
src/reqs.c |
Removes IE-specific POST handling and stops switching socket modes during relay. |
src/buffer.c |
Makes EAGAIN/EWOULDBLOCK propagate as errors rather than returning 0 bytes. |
Comments suppressed due to low confidence (1)
src/buffer.c:252
- This change makes EAGAIN/EWOULDBLOCK propagate as an error (instead of being treated like a recoverable/zero-byte read/write). Since this is intended to fix a timeout-driven infinite loop, it would be good to add a regression test in the existing test suite that exercises an idle socket hitting SO_RCVTIMEO/SO_SNDTIMEO and verifies the proxy breaks out/terminates the relay instead of spinning.
switch (errno) {
case EINTR:
bytesin = 0;
break;
default:
log_message (LOG_ERR,
"read_buffer: read() failed on fd %d: %s",
fd, strerror(errno));
bytesin = -1;
break;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /* | ||
| * Set the socket to non blocking -rjkaes | ||
| */ | ||
| int socket_nonblocking (int sock) | ||
| { | ||
| int flags; | ||
|
|
||
| assert (sock >= 0); | ||
|
|
||
| flags = fcntl (sock, F_GETFL, 0); | ||
| return fcntl (sock, F_SETFL, flags | O_NONBLOCK); | ||
| } | ||
|
|
||
| /* | ||
| * Set the socket to blocking -rjkaes | ||
| */ | ||
| int socket_blocking (int sock) | ||
| { | ||
| int flags; | ||
|
|
||
| assert (sock >= 0); | ||
|
|
||
| flags = fcntl (sock, F_GETFL, 0); | ||
| return fcntl (sock, F_SETFL, flags & ~O_NONBLOCK); | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| * Try to listen on one socket based on the addrinfo |
There was a problem hiding this comment.
The nonblocking helper functions were removed from sock.c, but sock.h still declares socket_nonblocking()/socket_blocking(). Even if nothing references them now, leaving stale extern declarations makes the socket API misleading; please remove these prototypes from sock.h (and any related documentation) to match the implementation.
|
tested locally for a week, seems fine. |
due to some ancient piece of code that's supposed to fix a bug in 1990's internet explorer, sockets were switched between blocking and non-blocking mode, making it hard to differentiate when socket timeouts kicked in.
with the IE bug workaround removed, sockets are now always in blocking mode so we no longer need to catch EAGAIN/EWOULDBLOCK and treat them specially, they now always are treated as an error (whenever they are returned, the timeout kicked in).
this should fix once and for all the cases where tinyproxy would not respect the user-provided socket timeouts, potentially causing endless loops.