Skip to content
Permalink
Browse files Browse the repository at this point in the history
libvncclient: handle half-open TCP connections
When a connection is not reset properly at the TCP level (e.g. sudden
power loss or process crash) the TCP connection becomes half-open and
read() always returns -1 with errno = EAGAIN while select() always
returns 0. This leads to an infinite loop and can be fixed by closing
the connection after a certain number of retries (based on a timeout)
has been exceeded.
  • Loading branch information
tobydox committed Apr 24, 2020
1 parent 323aa5e commit 5743301
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 2 deletions.
18 changes: 16 additions & 2 deletions libvncclient/sockets.c
Expand Up @@ -62,6 +62,8 @@ rfbBool errorMessageOnReadFailure = TRUE;
rfbBool
ReadFromRFBServer(rfbClient* client, char *out, unsigned int n)
{
const int USECS_WAIT_PER_RETRY = 100000;
int retries = 0;
#undef DEBUG_READ_EXACT
#ifdef DEBUG_READ_EXACT
char* oout=out;
Expand Down Expand Up @@ -151,10 +153,16 @@ ReadFromRFBServer(rfbClient* client, char *out, unsigned int n)
if (i <= 0) {
if (i < 0) {
if (errno == EWOULDBLOCK || errno == EAGAIN) {
if (client->readTimeout > 0 &&
++retries > (client->readTimeout * 1000 * 1000 / USECS_WAIT_PER_RETRY))
{
rfbClientLog("Connection timed out\n");
return FALSE;
}
/* TODO:
ProcessXtEvents();
*/
WaitForMessage(client, 100000);
WaitForMessage(client, USECS_WAIT_PER_RETRY);
i = 0;
} else {
rfbClientErr("read (%d: %s)\n",errno,strerror(errno));
Expand Down Expand Up @@ -194,10 +202,16 @@ ReadFromRFBServer(rfbClient* client, char *out, unsigned int n)
errno=WSAGetLastError();
#endif
if (errno == EWOULDBLOCK || errno == EAGAIN) {
if (client->readTimeout > 0 &&
++retries > (client->readTimeout * 1000 * 1000 / USECS_WAIT_PER_RETRY))
{
rfbClientLog("Connection timed out\n");
return FALSE;
}
/* TODO:
ProcessXtEvents();
*/
WaitForMessage(client, 100000);
WaitForMessage(client, USECS_WAIT_PER_RETRY);
i = 0;
} else {
rfbClientErr("read (%s)\n",strerror(errno));
Expand Down
1 change: 1 addition & 0 deletions libvncclient/vncviewer.c
Expand Up @@ -272,6 +272,7 @@ rfbClient* rfbGetClient(int bitsPerSample,int samplesPerPixel,
client->destPort = 5900;

client->connectTimeout = DEFAULT_CONNECT_TIMEOUT;
client->readTimeout = DEFAULT_READ_TIMEOUT;

client->CurrentKeyboardLedState = 0;
client->HandleKeyboardLedState = (HandleKeyboardLedStateProc)DummyPoint;
Expand Down
4 changes: 4 additions & 0 deletions rfb/rfbclient.h
Expand Up @@ -85,6 +85,7 @@
#define SERVER_PORT_OFFSET 5900

#define DEFAULT_CONNECT_TIMEOUT 60
#define DEFAULT_READ_TIMEOUT 0

#define DEFAULT_SSH_CMD "/usr/bin/ssh"
#define DEFAULT_TUNNEL_CMD \
Expand Down Expand Up @@ -454,6 +455,9 @@ typedef struct _rfbClient {
#endif
/* timeout in seconds for select() after connect() */
unsigned int connectTimeout;
/* timeout in seconds when reading from half-open connections in
* ReadFromRFBServer() - keep at 0 to disable timeout detection and handling */
unsigned int readTimeout;
} rfbClient;

/* cursor.c */
Expand Down

6 comments on commit 5743301

@mcatanzaro
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice some Linux distros are backporting this patch because it fixes CVE-2020-14398. However, this modifies the size of the rfbClient struct, which is public ABI. Accordingly, this patch is not suitable for backport unless dependent packages are rebuilt.

We need to be very careful with libvncserver because it isn't maintaining library soname properly; i.e. different releases of the library with the same soname actually contain ABI changes.

@bk138
Copy link
Member

@bk138 bk138 commented on 5743301 Nov 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcatanzaro please note that we're (now) very careful to just be adding to the end of the rfbClient struct, not removing members, not adding in between in order to preserver ABI compat. Thus, this patch is indeed suitable for backports.

@mcatanzaro
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @bk138, since this struct is public and likely to be allocated on the stack, I think you need to remove padding bytes from the end of the struct if you want to avoid breaking the ABI, yes? Unfortunately there is no padding here.

If #420 is implemented, this problem will go away.

@bk138
Copy link
Member

@bk138 bk138 commented on 5743301 Nov 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @bk138, since this struct is public and likely to be allocated on the stack, I think you need to remove padding bytes from the end of the struct if you want to avoid breaking the ABI, yes? Unfortunately there is no padding here.

It's always (at least that's the API) allocated on the heap, so the "internals" must be at the right place, but it doesn't do harm if it contains more members than expected its end.

If #420 is implemented, this problem will go away.

Not really, as struct members will still be there?

@mcatanzaro
Copy link

@mcatanzaro mcatanzaro commented on 5743301 Nov 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's always (at least that's the API) allocated on the heap, so the "internals" must be at the right place, but it doesn't do harm if it contains more members than expected its end.

Er... if the entire struct is declared in a public header, how can you guarantee it is always allocated on the heap? Example failure case is:

rfbClient client;
rfbInitClient(&client, argc, argv);

If the application is compiled against older libvncserver but run against new libvncserver, then boom! you've clobbered whatever is next on the stack.

Now I see that all of your example code uses rfbGetClient() to allocate the rfbClient. Hm, OK, so everything that uses that will all be fine, and I guess realistic applications are supposed to do that? Looking at the implementation of rfbGetClient(), I guess so, since that seems like a lot of work to configure it properly otherwise. But it still violates the normal rules distros expect for ABI stability, and it's probably(?) going to be caught by distro ABI checkers, and will likely result in downstreams refusing to take the patch regardless. So if you want to be able to add new members to a public struct, then please only declare the struct in a header file, and define it only in a source file such that its data members are all private, and provide getter/setter functions for whatever fields need to be accessible. Then there's no question that it's safe.

Not really, as struct members will still be there?

If the struct members are no longer public (I assume that's what is intended by "sealed") and the struct can no longer be allocated on the stack, then you can change whatever you want whenever you want without breaking ABI. Otherwise, you're going to scare downstreams like me. At least, I fear a struct size change more than I do denial of service.

@bk138
Copy link
Member

@bk138 bk138 commented on 5743301 Nov 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I see that all of your example code uses rfbGetClient() to allocate the rfbClient. Hm, OK, so everything that uses that will all be fine, and I guess realistic applications are supposed to do that? Looking at the implementation of rfbGetClient(), I guess so, since that seems like a lot of work to configure it properly otherwise. But it still violates the normal rules distros expect for ABI stability, and it's probably(?) going to be caught by distro ABI checkers, and will likely result in downstreams refusing to take the patch regardless. So if you want to be able to add new members to a public struct, then please only declare the struct in a header file, and define it only in a source file such that its data members are all private, and provide getter/setter functions for whatever fields need to be accessible. Then there's no question that it's safe.

Would love to have this. This is more or less what #420 is about.

Not really, as struct members will still be there?

If the struct members are no longer public (I assume that's what is intended by "sealed") and the struct can no longer be allocated on the stack, then you can change whatever you want whenever you want without breaking ABI. Otherwise, you're going to scare downstreams like me. At least, I fear a struct size change more than I do denial of service.

Sorry for scaring you, not my intention nor the original author's one, I guess. Let me propose #181 as a stop-gap solution.

Please sign in to comment.