From: Ian Jackson <ijackson@chiark.greenend.org.uk>
Date: Wed, 28 Jan 2015 23:17:03 +0000 (+0000)
Subject: Portability: Fix assumption about read() and write() on connecting sockets
X-Git-Tag: adns-1.5.1~12
X-Git-Url: http://www.chiark.greenend.org.uk/ucgi/~mdw/git/adns/commitdiff_plain/777763cb0994893c42f4030a442d3d44a403b8fa

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>
---

diff --git a/changelog b/changelog
index fedd31a..77eda68 100644
--- a/changelog
+++ b/changelog
@@ -3,6 +3,9 @@ adns (1.5.1~~) unstable; urgency=low
   * Portability fix for systems where socklen_t is bigger than int.
   * Fix for malicious optimisation of memcpy in test suite, which
     causes failure with gcc-4.1.9 -O3.  See Debian bug #772718.
+  * Fix TCP async connect handling.  The bug is hidden on Linux and on most
+    systems where the nameserver is on localhost.  If it is not hidden,
+    adns's TCP support is broken unless adns_if_noautosys is used.
 
  --
 
diff --git a/regress/case-1stservbroken.sys b/regress/case-1stservbroken.sys
index fe4363c..479ccb2 100644
--- a/regress/case-1stservbroken.sys
+++ b/regress/case-1stservbroken.sys
@@ -68,10 +68,13 @@ adnstest 1stservbroken
  select max=6 rfds=[4] wfds=[5] efds=[] to=13.987562
  select=1 rfds=[] wfds=[5] efds=[]
  +0.001172
+ select max=6 rfds=null wfds=[5] efds=null to=0.000000
+ select=1 rfds=null wfds=[5] efds=null
++0.000001
  read fd=5 buflen=1
  read=OK
      .
- +0.001161
+ +0.001160
  write fd=5
      003b311f 01000001 00000000 00000574 72756e63 04746573 74036977 6a0a7265
      6c617469 76697479 08677265 656e656e 64036f72 6702756b 00000c00 01.
@@ -104,9 +107,12 @@ adnstest 1stservbroken
  select max=6 rfds=[4] wfds=[5] efds=[] to=13.992868
  select=1 rfds=[] wfds=[5] efds=[]
  +0.001011
+ select max=6 rfds=null wfds=[5] efds=null to=0.000000
+ select=1 rfds=null wfds=[5] efds=null
++0.000001
  read fd=5 buflen=1
  read=EAGAIN
- +0.000595
+ +0.000594
  write fd=5
      003b311f 01000001 00000000 00000574 72756e63 04746573 74036977 6a0a7265
      6c617469 76697479 08677265 656e656e 64036f72 6702756b 00000c00 01.
diff --git a/regress/case-1stservtotcp.sys b/regress/case-1stservtotcp.sys
index e6ef382..87da98b 100644
--- a/regress/case-1stservtotcp.sys
+++ b/regress/case-1stservtotcp.sys
@@ -80,9 +80,12 @@ adnstest 1stservto
  select max=6 rfds=[4] wfds=[5] efds=[] to=13.991978
  select=1 rfds=[] wfds=[5] efds=[]
  +0.001038
+ select max=6 rfds=null wfds=[5] efds=null to=0.000000
+ select=1 rfds=null wfds=[5] efds=null
++0.000001
  read fd=5 buflen=1
  read=EAGAIN
- +0.001203
+ +0.001202
  write fd=5
      003b311f 01000001 00000000 00000574 72756e63 04746573 74036977 6a0a7265
      6c617469 76697479 08677265 656e656e 64036f72 6702756b 00000c00 01.
diff --git a/regress/case-2ndservtcp.sys b/regress/case-2ndservtcp.sys
index fa9e994..b3502c7 100644
--- a/regress/case-2ndservtcp.sys
+++ b/regress/case-2ndservtcp.sys
@@ -65,9 +65,12 @@ adnstest 2ndserver
  select max=6 rfds=[4] wfds=[5] efds=[] to=13.996770
  select=1 rfds=[] wfds=[5] efds=[]
  +1.-14443
+ select max=6 rfds=null wfds=[5] efds=null to=0.000000
+ select=1 rfds=null wfds=[5] efds=null
++0.000001
  read fd=5 buflen=1
  read=EHOSTUNREACH
- +0.000193
+ +0.000192
  close fd=5
  close=OK
  +0.000146
@@ -89,9 +92,12 @@ adnstest 2ndserver
  select max=6 rfds=[4] wfds=[5] efds=[] to=13.998787
  select=1 rfds=[] wfds=[5] efds=[]
  +0.000135
+ select max=6 rfds=null wfds=[5] efds=null to=0.000000
+ select=1 rfds=null wfds=[5] efds=null
++0.000001
  read fd=5 buflen=1
  read=EAGAIN
- +0.000062
+ +0.000061
  write fd=5
      003b311f 01000001 00000000 00000574 72756e63 04746573 74036977 6a0a7265
      6c617469 76697479 08677265 656e656e 64036f72 6702756b 00000c00 01.
diff --git a/regress/case-manyptrwrong.sys b/regress/case-manyptrwrong.sys
index 27c8cac..1dea912 100644
--- a/regress/case-manyptrwrong.sys
+++ b/regress/case-manyptrwrong.sys
@@ -125,9 +125,12 @@ adnstest ncipher -0x400
  select max=6 rfds=[4] wfds=[5] efds=[] to=1.987296
  select=1 rfds=[] wfds=[5] efds=[]
  +0.000949
+ select max=6 rfds=null wfds=[5] efds=null to=0.000000
+ select=1 rfds=null wfds=[5] efds=null
++0.000001
  read fd=5 buflen=1
  read=EAGAIN
- +0.000147
+ +0.000146
  write fd=5
      002b3123 01000001 00000000 00000332 35340130 02393903 32303307 696e2d61
      64647204 61727061 00000c00 01.
diff --git a/regress/case-manyptrwrongrem.sys b/regress/case-manyptrwrongrem.sys
index ccb3530..e427ffc 100644
--- a/regress/case-manyptrwrongrem.sys
+++ b/regress/case-manyptrwrongrem.sys
@@ -319,9 +319,12 @@ adnstest manyptrwrong -0x400
  select max=6 rfds=[4] wfds=[5] efds=[] to=13.539402
  select=1 rfds=[] wfds=[5] efds=[]
  +0.008807
+ select max=6 rfds=null wfds=[5] efds=null to=0.000000
+ select=1 rfds=null wfds=[5] efds=null
++0.000001
  read fd=5 buflen=1
  read=EAGAIN
- +0.000213
+ +0.000212
  write fd=5
      002b3123 01000001 00000000 00000332 35340130 02393903 32303307 696e2d61
      64647204 61727061 00000c00 01.
diff --git a/regress/case-manyptrwrongrst.sys b/regress/case-manyptrwrongrst.sys
index 80e8381..63fe0ef 100644
--- a/regress/case-manyptrwrongrst.sys
+++ b/regress/case-manyptrwrongrst.sys
@@ -403,9 +403,12 @@ adnstest default -0x400
  select max=6 rfds=[4] wfds=[5] efds=[] to=0.562305
  select=1 rfds=[] wfds=[5] efds=[]
  +0.001347
+ select max=6 rfds=null wfds=[5] efds=null to=0.000000
+ select=1 rfds=null wfds=[5] efds=null
++0.000001
  read fd=5 buflen=1
  read=EAGAIN
- +0.000274
+ +0.000273
  write fd=5
      002b3123 01000001 00000000 00000332 35340130 02393903 32303307 696e2d61
      64647204 61727061 00000c00 01.
diff --git a/regress/case-manyptrwrongrty.sys b/regress/case-manyptrwrongrty.sys
index 92be042..3116f80 100644
--- a/regress/case-manyptrwrongrty.sys
+++ b/regress/case-manyptrwrongrty.sys
@@ -164,9 +164,12 @@ adnstest default -0x400
  select max=6 rfds=[4] wfds=[5] efds=[] to=1.939813
  select=1 rfds=[] wfds=[5] efds=[]
  +0.001860
+ select max=6 rfds=null wfds=[5] efds=null to=0.000000
+ select=1 rfds=null wfds=[5] efds=null
++0.000001
  read fd=5 buflen=1
  read=EAGAIN
- +0.000211
+ +0.000210
  write fd=5
      002b3123 01000001 00000000 00000332 35340130 02393903 32303307 696e2d61
      64647204 61727061 00000c00 01.
diff --git a/regress/case-norecurse.sys b/regress/case-norecurse.sys
index f9509d7..16f07d3 100644
--- a/regress/case-norecurse.sys
+++ b/regress/case-norecurse.sys
@@ -109,9 +109,12 @@ adnstest default -0x416
  recvfrom fd=4 buflen=512
  recvfrom=EAGAIN
  +0.000116
+ select max=6 rfds=null wfds=[5] efds=null to=0.000000
+ select=1 rfds=null wfds=[5] efds=null
++0.000001
  read fd=5 buflen=1
  read=EAGAIN
- +0.000123
+ +0.000122
  write fd=5
      002b3123 01000001 00000000 00000134 03323034 02353003 31353807 696e2d61
      64647204 61727061 00000c00 01.
diff --git a/regress/case-norecurse2.sys b/regress/case-norecurse2.sys
index 2e2b8e5..32057b7 100644
--- a/regress/case-norecurse2.sys
+++ b/regress/case-norecurse2.sys
@@ -538,9 +538,12 @@ adnstest default -0x416
  select max=6 rfds=[4] wfds=[5] efds=[] to=13.987312
  select=1 rfds=[] wfds=[5] efds=[]
  +0.000364
+ select max=6 rfds=null wfds=[5] efds=null to=0.000000
+ select=1 rfds=null wfds=[5] efds=null
++0.000001
  read fd=5 buflen=1
  read=EAGAIN
- +0.000127
+ +0.000126
  write fd=5
      002b3123 01000001 00000000 00000134 03323034 02353003 31353807 696e2d61
      64647204 61727061 00000c00 01.
diff --git a/regress/case-tcpallfail.sys b/regress/case-tcpallfail.sys
index a49b1b9..5ddd0ef 100644
--- a/regress/case-tcpallfail.sys
+++ b/regress/case-tcpallfail.sys
@@ -25,9 +25,12 @@ adnstest anarres -0x400
  select max=6 rfds=[4] wfds=[5] efds=[] to=13.999272
  select=1 rfds=[] wfds=[5] efds=[]
  +0.000862
+ select max=6 rfds=null wfds=[5] efds=null to=0.000000
+ select=1 rfds=null wfds=[5] efds=null
++0.000001
  read fd=5 buflen=1
  read=ECONNREFUSED
- +0.000087
+ +0.000086
  close fd=5
  close=OK
  +0.000109
@@ -49,9 +52,12 @@ adnstest anarres -0x400
  select max=6 rfds=[4] wfds=[5] efds=[] to=13.999414
  select=1 rfds=[] wfds=[5] efds=[]
  +0.000339
+ select max=6 rfds=null wfds=[5] efds=null to=0.000000
+ select=1 rfds=null wfds=[5] efds=null
++0.000001
  read fd=5 buflen=1
  read=ECONNREFUSED
- +0.000075
+ +0.000074
  close fd=5
  close=OK
  +0.000084
diff --git a/regress/case-tcpblock.sys b/regress/case-tcpblock.sys
index 027df17..5dc1bdf 100644
--- a/regress/case-tcpblock.sys
+++ b/regress/case-tcpblock.sys
@@ -25,9 +25,12 @@ adnstest anarres -0x400
  select max=6 rfds=[4] wfds=[5] efds=[] to=13.999290
  select=1 rfds=[] wfds=[5] efds=[]
  +0.000937
+ select max=6 rfds=null wfds=[5] efds=null to=0.000000
+ select=1 rfds=null wfds=[5] efds=null
++0.000001
  read fd=5 buflen=1
  read=EAGAIN
- +0.000090
+ +0.000089
  write fd=5
      0035311f 01000001 00000000 00000474 65737403 69776a0a 72656c61 74697669
      74790867 7265656e 656e6403 6f726702 756b0000 010001.
diff --git a/regress/case-tcpblockbrk.sys b/regress/case-tcpblockbrk.sys
index 679d0d4..d22d17e 100644
--- a/regress/case-tcpblockbrk.sys
+++ b/regress/case-tcpblockbrk.sys
@@ -25,9 +25,12 @@ adnstest anarres -0x700
  select max=7 rfds=[5] wfds=[6] efds=[] to=13.996699
  select=1 rfds=[] wfds=[6] efds=[]
  +0.003661
+ select max=7 rfds=null wfds=[6] efds=null to=0.000000
+ select=1 rfds=null wfds=[6] efds=null
++0.000001
  read fd=6 buflen=1
  read=EAGAIN
- +0.000477
+ +0.000476
  write fd=6
      0035311f 01000001 00000000 00000474 65737403 69776a0a 72656c61 74697669
      74790867 7265656e 656e6403 6f726702 756b0000 010001.
@@ -125,9 +128,12 @@ adnstest anarres -0x700
  select max=7 rfds=[5] wfds=[6] efds=[] to=13.996686
  select=1 rfds=[] wfds=[6] efds=[]
  +0.000977
+ select max=7 rfds=null wfds=[6] efds=null to=0.000000
+ select=1 rfds=null wfds=[6] efds=null
++0.000001
  read fd=6 buflen=1
  read=ECONNREFUSED
- +0.000450
+ +0.000449
  close fd=6
  close=OK
  +0.000617
diff --git a/regress/case-tcpblockwr.sys b/regress/case-tcpblockwr.sys
index e232d28..07644f1 100644
--- a/regress/case-tcpblockwr.sys
+++ b/regress/case-tcpblockwr.sys
@@ -25,9 +25,12 @@ adnstest anarres -0x700
  select max=7 rfds=[5] wfds=[6] efds=[] to=13.996721
  select=1 rfds=[] wfds=[6] efds=[]
  +0.278976
+ select max=7 rfds=null wfds=[6] efds=null to=0.000000
+ select=1 rfds=null wfds=[6] efds=null
++0.000001
  read fd=6 buflen=1
  read=EAGAIN
- +0.001048
+ +0.001047
  write fd=6
      0035311f 01000001 00000000 00000474 65737403 69776a0a 72656c61 74697669
      74790867 7265656e 656e6403 6f726702 756b0000 010001.
diff --git a/regress/case-tcpbreakin.sys b/regress/case-tcpbreakin.sys
index d20b0d9..e64e26f 100644
--- a/regress/case-tcpbreakin.sys
+++ b/regress/case-tcpbreakin.sys
@@ -25,9 +25,12 @@ adnstest default
  select max=6 rfds=[4] wfds=[5] efds=[] to=13.997928
  select=1 rfds=[] wfds=[5] efds=[]
  +0.000536
+ select max=6 rfds=null wfds=[5] efds=null to=0.000000
+ select=1 rfds=null wfds=[5] efds=null
++0.000001
  read fd=5 buflen=1
  read=EAGAIN
- +0.000118
+ +0.000117
  write fd=5
      002a311f 01000001 00000000 00000136 02343502 31380331 37320769 6e2d6164
      64720461 72706100 000c0001.
@@ -81,9 +84,12 @@ adnstest default
  select max=6 rfds=[4] wfds=[5] efds=[] to=13.997928
  select=1 rfds=[] wfds=[5] efds=[]
  +0.000536
+ select max=6 rfds=null wfds=[5] efds=null to=0.000000
+ select=1 rfds=null wfds=[5] efds=null
++0.000001
  read fd=5 buflen=1
  read=EAGAIN
- +0.000723
+ +0.000722
  write fd=5
      00353120 01000001 00000000 00000864 6176656e 616e740a 72656c61 74697669
      74790867 7265656e 656e6403 6f726702 756b0000 010001.
diff --git a/regress/case-tcpmultipart.sys b/regress/case-tcpmultipart.sys
index 8f59ea9..37cc29c 100644
--- a/regress/case-tcpmultipart.sys
+++ b/regress/case-tcpmultipart.sys
@@ -25,9 +25,12 @@ adnstest tunnel
  select max=6 rfds=[4] wfds=[5] efds=[] to=13.998324
  select=1 rfds=[] wfds=[5] efds=[]
  +1.-647444
+ select max=6 rfds=null wfds=[5] efds=null to=0.000000
+ select=1 rfds=null wfds=[5] efds=null
++0.000001
  read fd=5 buflen=1
  read=EAGAIN
- +0.000176
+ +0.000175
  write fd=5
      002d311f 01000001 00000000 00000331 33320237 36033232 34033139 3507696e
      2d616464 72046172 70610000 0c0001.
diff --git a/regress/case-tcpptr.sys b/regress/case-tcpptr.sys
index 662689c..14fc6a4 100644
--- a/regress/case-tcpptr.sys
+++ b/regress/case-tcpptr.sys
@@ -25,9 +25,12 @@ adnstest default
  select max=6 rfds=[4] wfds=[5] efds=[] to=13.997928
  select=1 rfds=[] wfds=[5] efds=[]
  +0.000536
+ select max=6 rfds=null wfds=[5] efds=null to=0.000000
+ select=1 rfds=null wfds=[5] efds=null
++0.000001
  read fd=5 buflen=1
  read=EAGAIN
- +0.000118
+ +0.000117
  write fd=5
      002a311f 01000001 00000000 00000136 02343502 31380331 37320769 6e2d6164
      64720461 72706100 000c0001.
diff --git a/regress/update-extra-select b/regress/update-extra-select
new file mode 100755
index 0000000..0325309
--- /dev/null
+++ b/regress/update-extra-select
@@ -0,0 +1,27 @@
+#!/usr/bin/perl -wp
+use strict;
+
+our ($fd, $timeadj);
+
+BEGIN {
+    $fd = -1;
+}
+
+if (m/^ connect fd=(\d+) /) {
+    $fd=$1; next;
+}
+
+if (m/^ read fd=$fd buflen=1$/) {
+    die if $timeadj;
+    print
+	" select max=".($fd+1)." rfds=null wfds=[$fd]".
+	" efds=null to=0.000000\n".
+	" select=1 rfds=null wfds=[$fd] efds=null\n".
+	"+0.000001\n";
+    $timeadj = -0.000001;
+}
+
+if (m/^ \+([0-9.]+)$/ && $timeadj) {
+    $_ = sprintf " +%.6f\n", $1 + $timeadj;
+    $timeadj = 0;
+}
diff --git a/src/event.c b/src/event.c
index eec4b6a..39b67b7 100644
--- a/src/event.c
+++ b/src/event.c
@@ -444,6 +444,22 @@ int adns_processwriteable(adns_state ads, int fd, const struct timeval *now) {
     assert(ads->tcprecv.used==0);
     assert(ads->tcprecv_skip==0);
     for (;;) {
+      /* This function can be called even if the fd wasn't actually
+       * flagged as writeable.  For asynch tcp connect we have to
+       * actually use the writeability to tell us the connect has
+       * completed (or failed), so we need to double check. */
+      fd_set writeable;
+      struct timeval timeout = { 0,0 };
+      FD_ZERO(&writeable);
+      FD_SET(ads->tcpsocket,&writeable);
+      r= select(ads->tcpsocket+1,0,&writeable,0,&timeout);
+      if (r==0) break;
+      if (r<0) {
+	if (errno==EINTR) continue;
+	adns__tcp_broken(ads,"select","failed connecting writeability check");
+	r= 0; goto xit;
+      }
+      assert(FD_ISSET(ads->tcpsocket,&writeable));
       if (!adns__vbuf_ensure(&ads->tcprecv,1)) { r= ENOMEM; goto xit; }
       r= read(ads->tcpsocket,&ads->tcprecv.buf,1);
       if (r==0 || (r<0 && (errno==EAGAIN || errno==EWOULDBLOCK))) {