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 fedd31a545455b6e584cce04511c91d1503b744f..77eda6820b093ad6eaf87f1004a871d043d51c3c 100644 (file)
--- 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 fe4363ce41cda6995263d68209f3cdcc0d64623f..479ccb246843679630fa171fc03f2ddf883cf722 100644 (file)
--- 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 e6ef382abc754a0851ef7b96f7141c0d13135d43..87da98be7f55eaa6c3a34c11f86c8500c31540ce 100644 (file)
--- 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 fa9e9941c39c1b4022435edf811de7a558e4c7b2..b3502c7b48e99f3a04283ed5dbf26ae4902e28e3 100644 (file)
--- 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 27c8cacec1d3494d0df7fa94cc27e40788da18d2..1dea912e172129998660fff41d5f50cad080071a 100644 (file)
--- 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 ccb353068e61448b72fb2f4f3d911d5671896fd7..e427ffce7c5bc0351e5d83ed571b2d2849e19a0e 100644 (file)
--- 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 80e8381e984a80abf58444ca0434fb247250cfa9..63fe0efec22805f76f8915e0ec3edd0b94ec9c6d 100644 (file)
--- 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 92be042ab75ba657621d6baf6532d5b006c0f0c1..3116f80ae96887dcf7df3ac9ae81ba955d9d7b30 100644 (file)
--- 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 f9509d7647c61416c7d6c7d31849a425b1ea544e..16f07d3d6c787d3ff20aaf47ec19a9822887536f 100644 (file)
--- 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 2e2b8e5ac2fed47c985a54c020984d6eb95ee146..32057b7ec28d1477c736ac678e1fff3387cb9e86 100644 (file)
--- 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 a49b1b9f236b7a7dcefcd4d97b3d10b458fd5ba1..5ddd0ef00494e0d4b19301a59768129f96af36d1 100644 (file)
--- 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 027df17bbb3295fb168a16b28791916bd07ded85..5dc1bdfb26ee8b6777aa161443ca0661febbced4 100644 (file)
--- 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 679d0d490abdc4fbe40ec13968ae5f9039eef26d..d22d17e7b2725925b2f99985adc2f1faf2016e57 100644 (file)
--- 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 e232d2832137123a248c2d3e1dfda6c92e885613..07644f117f8fba26346d5de0496a89b7ae1ef260 100644 (file)
--- 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 d20b0d9f5b0150799d574e6490e57c6cc6ffed9a..e64e26f9ba1546ff4c3dd1d2acebcbdfbda3d02e 100644 (file)
--- 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 8f59ea9e409ed26ed645873eca8249d3a3831ae8..37cc29cb72f8c7b8c23ffeefa2ab642c1c77ae80 100644 (file)
--- 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 662689ca622e1f4a691a80b08a0782c549b724b7..14fc6a4006b9228c01679e37dd2d0e4308958bb0 100644 (file)
--- 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 (executable)
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 eec4b6a3d8724432ef42be6ad7732f7ab3474e24..39b67b75ad6d025538d233f245e98ae1812c4660 100644 (file)
--- 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))) {