chiark / gitweb /
Portability: Fix assumption about read() and write() on connecting sockets
authorIan Jackson <ijackson@chiark.greenend.org.uk>
Wed, 28 Jan 2015 23:17:03 +0000 (23:17 +0000)
committerIan Jackson <ijackson@chiark.greenend.org.uk>
Thu, 29 Jan 2015 00:45:57 +0000 (00:45 +0000)
commit777763cb0994893c42f4030a442d3d44a403b8fa
tree3e7a1dd4b9e8a64ac6202443461f706eea857fa4
parent47b9db5941ba11277d7bd62060057202236fb921
Portability: Fix assumption about read() and write() on connecting sockets

Without adns_if_noautosys, adns would attempt read() on its TCP client
socket immediately after getting EINPROGRESS from connect(), and
assume that EAGAIN/EWOULDBLOCK means the socket is connected.

This is actually not correct on any platform that I'm aware of.
However, on Linux, write() on a socket which is being connected
returns EAGAIN, so everything seemed to work - adns would think that
the socket's window was full and wait for it to become writeable.  On
many other platforms, write() on such a socket returns ENOTCONN.

The result is that adns's TCP support may fail to work properly on
such platforms, especially if the nameserver is not localhost.  (A
connect to a suitable server on localhost often completes immediately,
which avoids exposing the bug.)  adns might fail to be able to do TCP
at all.

We detect the completion (successful or otherwise) of connect() by
selecting the fd (as contemplated by SuS).  This might expose us to
spurious fd writeability indications, if such things exist, but the
nonblocking connect API demands that they don't at least in this case.

We always do this select check, in adns_processwriteable.  In theory
this is sometimes unnecessary, because adns_processwriteable's caller
has probably just got a writeability indication from poll or select.
But adns should not assume that its caller will never feed it spurious
events, and there seems little point optimising away one syscall per
tcp connection (given that adns reuses the connection where possible.)

This behaviour naturally causes a lot of the regression tests to fail.
So in this commit we also update all the regression tests.  This has
been done in a programmatic way, by running
   perl -i ./update-extra-select case-*.sys

update-extra-select is supplied in this commit.  It is a (hopefully
easy to understand) script which adds a select just before every
applicable read.  The pretended select always reports that the fd is
useable, which is what would justify adns's subsequent behaviour.

(Most of the tests run with adns_if_noautosys - since that's the
default and has to be toggled off - and in those cases the socket has
in any case just been reported as writeable.)

Note that regress/update-extra-select is not idempotent.  Here, it has
been used exactly once.  (It is going to be deleted again in the next
commit.)

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
19 files changed:
changelog
regress/case-1stservbroken.sys
regress/case-1stservtotcp.sys
regress/case-2ndservtcp.sys
regress/case-manyptrwrong.sys
regress/case-manyptrwrongrem.sys
regress/case-manyptrwrongrst.sys
regress/case-manyptrwrongrty.sys
regress/case-norecurse.sys
regress/case-norecurse2.sys
regress/case-tcpallfail.sys
regress/case-tcpblock.sys
regress/case-tcpblockbrk.sys
regress/case-tcpblockwr.sys
regress/case-tcpbreakin.sys
regress/case-tcpmultipart.sys
regress/case-tcpptr.sys
regress/update-extra-select [new file with mode: 0755]
src/event.c