Skip to content

libvncclient: use poll() if available (re-land of #528, repeater regression fixed) - #738

Open
Juern-Univention wants to merge 2 commits into
LibVNC:masterfrom
Juern-Univention:use-poll
Open

Juern-Univention wants to merge 2 commits into
LibVNC:masterfrom
Juern-Univention:use-poll

Conversation

@Juern-Univention

Copy link
Copy Markdown

This re-lands #528 / 993df68, which was reverted in 7dd2750 because it broke
connecting a LibVNCServer-based server to an RFB repeater
(bk138/droidVNC-NG#109). That regression and two further defects in the
original patch are fixed here.

Why

select() cannot handle file descriptors >= FD_SETSIZE (1024 on most
systems): FD_SET() writes past the fd_set. It does not fail loudly — in a
run with a socket on fd 1035 the stray write landed in the neighbouring
fd_set on the stack, so the socket was never registered and the connect wait
simply timed out:

socket(AF_INET, SOCK_STREAM, IPPROTO_TCP) = 1035
connect(1035, ...) = -1 EINPROGRESS
pselect6(1036, NULL, [], [1028 1035], {tv_sec=60}, NULL) = 0 (Timeout)

Note the empty write set. A client whose socket gets a high number cannot
connect at all, which is what large deployments run into.

What was wrong with the original patch

  • sock_wait_for_connected() waited for POLLIN|POLLPRI. Completion of a
    non-blocking connect() is signalled as writability. Against a peer that
    stays silent until spoken to — an UltraVNC repeater in mode 2 reads a 250
    byte id before it answers — the wait ran out the full rfbMaxClientWait, so
    the caller closed the connection before sending the id, and the repeater
    reported the missing id as Incorrect id. Now waits for POLLOUT.
  • WaitForMessage() truncated its microsecond timeout to milliseconds, so
    any wait below 1 ms became a non-blocking poll() and turned callers that
    wait briefly into busy loops. The conversion now rounds up.
  • pfd.revents was read even when poll() returned 0 or -1, i.e. when it
    had not been filled in. It is now only inspected on a positive return, and
    POLLERR/POLLHUP are treated the way select() behaves: reported as
    ready, so the following read()/write() drains what is left and then
    reports the error.

Additionally the invalid-socket check applies to both implementations again,
and the poll() code is gated on LIBVNCSERVER_HAVE_POLL and
LIBVNCSERVER_HAVE_POLL_H, so a platform with the symbol but not the header
cannot end up calling poll() unprototyped.

Testing

test/repeatertest.c (second commit) covers the regression that caused the
revert: it stands in for the repeater with a listening socket on a
kernel-picked loopback port and checks the connection is established without
waiting out rfbMaxClientWait, that the id arrives in full, and that the
ProtocolVersion message follows.

build repeater test high-fd connect + RFB round trip 200 x WaitForMessage(999us)
this PR pass fd 1035, 133 ms, update received 215 ms
poll() forced off (select fallback) pass n/a 216 ms
master pass fails after 60 s 216 ms
993df68 as reverted fails after 20 s fd 1035, 130 ms 0 ms (spins)

Also verified: WriteToRFBServer() of 8 MB over a slow-draining non-blocking
socket takes the poll(POLLOUT) path 42 times (strace) and delivers every
byte, in both the poll and select builds; both #ifdef branches compile
warning-free under -Wall -Wextra; full ctest suite passes (5/5).

select() cannot handle file descriptors >= FD_SETSIZE (1024 on most
systems): FD_SET() then writes past the fd_set. The stray write lands in
whatever follows it, so the socket is never registered and the wait times
out instead of failing loudly -- a client whose socket got a high number
cannot connect at all. Use poll() where both the function and its header
are available and keep select() as the fallback.

Based on 993df68 by Tobias Junghans,
which was reverted in 7dd2750 because it
broke connecting a LibVNCServer-based server to an RFB repeater. That
regression is fixed here, along with two further defects:

* sock_wait_for_connected() waited for POLLIN|POLLPRI, while completion
  of a non-blocking connect() is signalled as writability. Against a peer
  that stays silent until spoken to -- an UltraVNC repeater in mode 2
  reads a 250 byte id before it says anything -- the wait ran into the
  full rfbMaxClientWait timeout, so the caller gave up and closed the
  connection before sending that id, which the repeater in turn reported
  as "Incorrect id" (bk138/droidVNC-NG#109). It waits for POLLOUT now.

* WaitForMessage() truncated its microsecond timeout to milliseconds, so
  any wait shorter than 1 ms became a non-blocking poll() and turned
  callers that wait briefly into busy loops. The conversion rounds up.

* pfd.revents was read even when poll() returned 0 or -1, i.e. when it
  had not been filled in. It is only inspected on a positive return now,
  and POLLERR/POLLHUP are handled the way select() behaves: reported as
  ready, so that the following read()/write() drains what is left and
  then reports the error.

Also let the invalid-socket check apply to both implementations again,
and gate the poll() code on LIBVNCSERVER_HAVE_POLL *and*
LIBVNCSERVER_HAVE_POLL_H so that a platform which has the symbol but not
the header cannot end up calling poll() unprototyped.
Covers rfbConnectToTcpAddr() -> sock_wait_for_connected() against a peer
that says nothing until it has been spoken to, which is what broke when
poll() was first introduced in 993df68 and what got that commit reverted
in 7dd2750: an UltraVNC repeater in mode 2 reads a 250 byte id before it
answers, so a connect completion wait that waits for readability instead
of writability never fires.

The test stands in for the repeater with a plain listening socket on a
kernel-picked loopback port and checks that the connection is established
without waiting out rfbMaxClientWait, that the id arrives in full and that
the ProtocolVersion message follows it. It is UNIX-only since it speaks
BSD sockets directly, and carries a ctest timeout so that a regression
fails the run rather than stalling it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants