Subject: Re: Outstanding resolver bug PR 6410
To: None <itojun@iijlab.net>
From: maximum entropy <entropy@zippy.bernstein.com>
List: current-users
Date: 07/26/2000 14:17:46
>From: itojun@iijlab.net
>Date: Thu, 27 Jul 2000 00:01:34 +0900
>
>>	Your patch: set seconds to 0 when seconds < 0, 
>>	BIND 4.9.7 and 8.2.2: set seconds to 1 when seconds <= 0, 

Either behavior is OK with me.  The important idea is that the
requested timeout must eventually take place.  Hanging indefinitely is
not OK.

>	I checked the current tree, and found that our code is exactly the
>	same as BIND 4.9.7 and BIND 8.2.3.  so I must ask.

The code in src/dist/bind/lib/libresolv is OK and contains the fix
that Vixie implemented based on my report (and a similar report from
Chrisos Zoulas).  If that's the code that we use to build libresolv.a,
then libresolv.a probably works correctly.  It's the resolver in libc
that has the bug described in my PR...sorry if that wasn't clear in my
most recent posting.

>>>This bug was also reported to the BIND development team at the same
>>>time.  They quickly acknowledged the problem and fixed it immediately
>>>in their distribution.
>
>	... which BIND revision contains your proposed fix?

See above.

Attached below is an EDITED copy of the message I received from Paul
Vixie when he implemented this fix.  I have edited it to remove some
comments that it might be inappropriate for me to repeat here.

Summary-line: 13-Sep             vixie@mibh.net [188] #Re: [christos@zoulas.com: Re: lib/6410] 
Mail-from: From vixie@mibh.net Mon Sep 13 21:01:34 1999
	by tardis.bernstein.com (8.8.8/8.8.8) with ESMTP id VAA08081
	for <entropy@tardis.bernstein.com>; Mon, 13 Sep 1999 21:01:31 -0400 (EDT)
	by ib.rc.vix.com (8.9.1/8.9.1) via ESMTP id RAA02728; Mon, 13 Sep 1999 17:50:20 -0700 (PDT)
	env-from (vixie@mibh.net)
	by bb.rc.vix.com (8.9.1/8.9.1) via ESMTP id RAA20037; Mon, 13 Sep 1999 17:50:20 -0700 (PDT)
	env-from (vixie@mibh.net)
Message-Id: <199909140050.RAA20037@bb.rc.vix.com>
To: maximum entropy <entropy@tardis.bernstein.com>
cc: christos@zoulas.com
Subject: Re: [christos@zoulas.com: Re: lib/6410] 
In-reply-to: Your message of "Thu, 09 Sep 1999 18:10:08 EDT."
             <199909092210.SAA02194@tardis.bernstein.com> 
Date: Mon, 13 Sep 1999 17:50:06 -0700
From: Paul A Vixie <vixie@mibh.net>

To: maximum entropy <entropy@tardis.bernstein.com>
cc: christos@zoulas.com
Subject: Re: [christos@zoulas.com: Re: lib/6410] 
In-reply-to: Your message of "Thu, 09 Sep 1999 18:10:08 EDT."
             <199909092210.SAA02194@tardis.bernstein.com> 
Date: Mon, 13 Sep 1999 17:50:06 -0700
From: Paul A Vixie <vixie@mibh.net>

thanks for your report.  it turns out that the netbsd resolver is
significantly out of date in that it lacks the thread safety of
modern bind and also does not have exponential backoff behaviour
on retransmissions.  [... REMARKS DELETED ...]

i'm going to try to isolate the behaviour you want as a patch on
top of christos' patch.  the basic bug here is not checking for
EINTR after select or poll, and it's not specific to linux or java
since any signal handler can make select or poll return early, thus
needing this timer recomputation.  thank you both for pointing this
out.

Index: res_send.c
===================================================================
RCS file: /proj/cvs/isc/bind/src/lib/resolv/res_send.c,v
retrieving revision 8.31
diff -u -r8.31 res_send.c
--- res_send.c	1999/08/24 05:39:24	8.31
+++ res_send.c	1999/09/14 00:47:26
@@ -93,17 +93,26 @@
 #include <errno.h>
 #include <netdb.h>
 #include <resolv.h>
+#include <signal.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
 
+#include <isc/eventlib.h>
+
 #include "port_after.h"
 
 /* Options.  Leave them on. */
 #define DEBUG
 #include "res_debug.h"
 
+#ifdef NEED_PSELECT
+static int		pselect(int, void *, void *, void *,
+				struct timespec *,
+				const sigset_t *);
+#endif
+
 #define	CHECK_SRVR_ADDR
 		
 #ifdef DEBUG
@@ -511,10 +520,10 @@
 			/*
 			 * Use datagrams.
 			 */
-			struct timeval timeout;
+			struct timespec start, timeout, finish;
 			fd_set dsmask;
 			struct sockaddr_in from;
-			int fromlen, waited;
+			int fromlen, seconds;
 
 			if (statp->_sock < 0 ||
 			    (statp->_flags & RES_F_VC) != 0) {
@@ -636,54 +645,51 @@
 			}
 #endif /* !CANNOT_CONNECT_DGRAM */
 
-			/*
-			 * Wait for reply
-			 */
-			timeout.tv_sec = (statp->retrans << try);
-			if (try > 0)
-				timeout.tv_sec /= statp->nscount;
-			if ((long) timeout.tv_sec <= 0)
-				timeout.tv_sec = 1;
-			timeout.tv_usec = 0;
-			waited = 0;
- wait:
 			if (statp->_sock < 0 || statp->_sock > highestFD) {
 				Perror(statp, stderr,
 				       "fd out-of-bounds", EMFILE);
 				res_nclose(statp);
 				goto next_ns;
 			}
+
+			/*
+			 * Wait for reply
+			 */
+			seconds = (statp->retrans << try);
+			if (try > 0)
+				seconds /= statp->nscount;
+			if (seconds <= 0)
+				seconds = 1;
+			start = evNowTime();
+			timeout = evConsTime(seconds, 0);
+			finish = evAddTime(start, timeout);
+ wait:
 			FD_ZERO(&dsmask);
 			FD_SET(statp->_sock, &dsmask);
-			n = select(statp->_sock + 1, &dsmask, (fd_set *)NULL,
-				   (fd_set *)NULL, &timeout);
+			n = pselect(statp->_sock + 1,
+				    &dsmask, NULL, NULL,
+				    &timeout, NULL);
+			if (n == 0) {
+				Dprint(statp->options & RES_DEBUG,
+				       (stdout, ";; timeout\n"));
+				gotsomewhere = 1;
+				goto next_ns;
+			}
 			if (n < 0) {
 				if (errno == EINTR) {
-					/*
-					 * On systems that implement user
-					 * threads (java), we cannot keep
-					 * looping, because if it happens
-					 * that our packet gets lost, we'll
-					 * be looping forever because the
-					 * scheduler we'll keep sending us
-					 * SIGALRM [christos 19990815]
-					 */
-					if (waited++ > 3)
+					struct timespec now;
+
+					now = evNowTime();
+					if (evCmpTime(finish, now) >= 0) {
+						timeout = evSubTime(finish,
+								    now);
 						goto wait;
+					}
 				}
 				Perror(statp, stderr, "select", errno);
 				res_nclose(statp);
 				goto next_ns;
 			}
-			if (n == 0) {
-				/*
-				 * timeout
-				 */
-				Dprint(statp->options & RES_DEBUG,
-				       (stdout, ";; timeout\n"));
-				gotsomewhere = 1;
-				goto next_ns;
-			}
 			errno = 0;
 			fromlen = sizeof(struct sockaddr_in);
 			resplen = recvfrom(statp->_sock, (char*)ans, anssiz,0,
@@ -858,3 +864,30 @@
 		(a1->sin_port == a2->sin_port) &&
 		(a1->sin_addr.s_addr == a2->sin_addr.s_addr));
 }
+
+#ifdef NEED_PSELECT
+/* XXX needs to move to the porting library. */
+static int
+pselect(int nfds, void *rfds, void *wfds, void *efds,
+	struct timespec *tsp,
+	const sigset_t *sigmask)
+{
+	struct timeval tv, *tvp;
+	sigset_t sigs;
+	int n;
+
+	if (tsp) {
+		tvp = &tv;
+		tv = evTimeVal(*tsp);
+	} else
+		tvp = NULL;
+	if (sigmask)
+		sigprocmask(SIG_SETMASK, sigmask, &sigs);
+	n = select(nfds, rfds, wfds, efds, tvp);
+	if (sigmask)
+		sigprocmask(SIG_SETMASK, &sigs, NULL);
+	if (tsp)
+		*tsp = evTimeSpec(tv);
+	return (n);
+}
+#endif


--
entropy -- it's not just a good idea, it's the second law.