Bug 16086

Comment 1 Sheldon Hearn 2000-01-13 12:54:25 UTC

Responsible Changed
From-To: freebsd-bugs->des

Over to the inetd(8) maintainer, although he may defer to 
the author of this particular builtin function. 

Additional problem: neither the original code nor the submitted patch
handle EINTR properly.

DES
-- 
Dag-Erling Smorgrav - des@flood.ping.uio.no

> Additional problem: neither the original code nor the submitted patch
> handle EINTR properly.
 
Is the interupt comming from the alarm() in the main inetd code?
This should probably be disabeled when the process forks just before
the signal handlers are reset? (Patch at bottom).

I can confirm the problem with tcpwrappers not being able to use
the builtin version of ident too. I wonder if it is worth making
libwrap send one packet in the name of "efficency", once the inetd
problem is fixed. This seems to be the code in question:

                /*
                 * Send query to server. Neglect the risk that a 13-byte
                 * write would have to be fragmented by the local system and
                 * cause trouble with buggy System V stdio libraries.
                 */

                fprintf(fp, "%u,%u\r\n",
                        ntohs(rmt_sin->sin_port),
                        ntohs(our_sin->sin_port));
                fflush(fp);

It would be pretty easy to replace this with an asprintf+write or
something.

        David.


--- inetd.c.orig	Fri Jan 28 22:40:03 2000
+++ inetd.c	Fri Jan 28 22:40:16 2000
@@ -632,6 +632,7 @@
 				for (tmpint = maxsock; tmpint > 2; tmpint--)
 					if (tmpint != ctrl)
 						(void) close(tmpint);
+				alarm(0);
 				sigaction(SIGALRM, &saalrm, (struct sigaction *)0);
 				sigaction(SIGCHLD, &sachld, (struct sigaction *)0);
 				sigaction(SIGHUP, &sahup, (struct sigaction *)0);

There seemed to be a problem with the originally quoted patch, in
that it didn't respect the timeout set anymore. I've included a
patch that uses Hajimu UMEMOTO's code to reassemble multiple packet
requests, but sets a timer before starting.

With this patch the process just exits without calling iderror. I
did this because this method is used to impliment timeouts for the
tcpmux service further down the file. I guess it would be possible
to catch the signal, but it hardly seems worth it. As interupting
the code is now the desired effect I think EINTR handling shouldn't
be an issue now?

I've also included a patch for the ident code in libwrap which
makes it send one packet requests. (The file is in contrib/tcp_wrappers).
It seems clear from the surounding code that fragmenting the request
wasn't a desirable effect.

	David.

--- builtins.c	2000/01/25 14:52:09	1.17
+++ builtins.c	2000/01/29 15:13:57
@@ -342,6 +342,8 @@
 		10,
 		0
 	};
+	struct itimerval it;
+	struct sigaction sa;
 	struct passwd *pw = NULL;
 	fd_set fdset;
 	char buf[BUFSIZE], *cp = NULL, *p, **av, *osname = NULL, garbage[7];
@@ -349,6 +351,7 @@
 	int len, c, fflag = 0, nflag = 0, rflag = 0, argc = 0, usedfallback = 0;
 	int gflag = 0, Rflag = 0, getcredfail = 0;
 	u_short lport, fport;
+	int bufsiz = sizeof(buf) - 1, size = 0;
 
 	inetd_setproctitle(sep->se_service, s);
 	/*
@@ -448,18 +451,29 @@
 	 * "local_port , foreign_port\r\n" (with local being the
 	 * server's port and foreign being the client's.)
 	 */
-	FD_ZERO(&fdset);
-	FD_SET(s, &fdset);
-	if (select(s + 1, &fdset, NULL, NULL, &tv) == -1)
-		iderror(0, 0, s, errno);
-	if (ioctl(s, FIONREAD, &len) == -1)
-		iderror(0, 0, s, errno);
-	if (len >= sizeof(buf))
-		len = sizeof(buf) - 1;
-	len = read(s, buf, len);
-	if (len == -1)
-		iderror(0, 0, s, errno);
-	buf[len] = '\0';
+	sa.sa_flags = 0;
+	sigemptyset(&sa.sa_mask);
+	sa.sa_handler = SIG_DFL;
+	sigaction(SIGALRM, &sa, (struct sigaction *)0);
+	timerclear(&(it.it_interval));
+	it.it_value = tv;
+	setitimer(ITIMER_REAL, &it, NULL);
+	do {
+		FD_ZERO(&fdset);
+		FD_SET(s, &fdset);
+		if (select(s + 1, &fdset, NULL, NULL, NULL) == -1)
+			iderror(0, 0, s, errno);
+		if (ioctl(s, FIONREAD, &len) == -1)
+			iderror(0, 0, s, errno);
+		if (len > bufsiz)
+			len = bufsiz;
+		len = read(s, &buf[size], len);
+		if (len == -1)
+			iderror(0, 0, s, errno);
+		bufsiz -= len;
+		size += len;
+	} while ((size < 1 || buf[size - 1] != '\n') && bufsiz > 0);
+	buf[size] = '\0';
 	if (sscanf(buf, "%hu , %hu", &lport, &fport) != 2)
 		iderror(0, 0, s, 0);
 	if (gflag) {


--- rfc931.c.orig	Sun Mar 14 17:13:19 1999
+++ rfc931.c	Sat Jan 29 15:32:06 2000
@@ -128,9 +128,10 @@
 		 * cause trouble with buggy System V stdio libraries.
 		 */
 
-		fprintf(fp, "%u,%u\r\n",
+		snprintf(buffer, sizeof(buffer), "%u,%u\r\n",
 			ntohs(rmt_sin->sin_port),
 			ntohs(our_sin->sin_port));
+		fwrite(buffer, strlen(buffer), 1, fp);
 		fflush(fp);
 
 		/*

I've cleaned up the patch for inetd's builtin ident service so it
applies to -current. This fixes the problem where the ident service
will not read a query unless it arrives in a single packet. Our
libwrap sends such requests, so this is worth fixing. I've also
updated the patch for libwrap to make it only send a single packet.

This patch also fixes a problem where it didn't check it had recieved
all of the second port number, so it was possible (but unlikely)
that a reqest for 23, 11 would be delt with as "23, 1". It now
checks for a non-digit delimiting character after the second port
number. (The rfc seems to say it should be a space or \r, but
there is probably no harm in being tolerant).

Is there any chance this can be added to -current in time for
merging to 4.1?

	David.


Index: builtins.c
===================================================================
RCS file: /cvs/FreeBSD-CVS/src/usr.sbin/inetd/builtins.c,v
retrieving revision 1.23
diff -u -r1.23 builtins.c
--- builtins.c	2000/05/30 22:51:05	1.23
+++ builtins.c	2000/07/08 22:50:57
@@ -344,13 +344,15 @@
 		10,
 		0
 	};
+	struct itimerval it;
+	struct sigaction sa;
 	struct passwd *pw = NULL;
 	fd_set fdset;
-	char buf[BUFSIZE], *cp = NULL, *p, **av, *osname = NULL, garbage[7];
+	char buf[BUFSIZE], *cp = NULL, *p, **av, *osname = NULL, garbage[7], e;
 	char *fallback = NULL;
 	socklen_t socklen;
 	ssize_t ssize;
-	size_t size;
+	size_t size, bufsiz;
 	int c, fflag = 0, nflag = 0, rflag = 0, argc = 0, usedfallback = 0;
 	int gflag = 0, getcredfail = 0, onreadlen;
 	u_short lport, fport;
@@ -450,19 +452,33 @@
 	 * "local_port , foreign_port\r\n" (with local being the
 	 * server's port and foreign being the client's.)
 	 */
-	FD_ZERO(&fdset);
-	FD_SET(s, &fdset);
-	if (select(s + 1, &fdset, NULL, NULL, &tv) == -1)
-		iderror(0, 0, s, errno);
-	if (ioctl(s, FIONREAD, &onreadlen) == -1)
-		iderror(0, 0, s, errno);
-	if (onreadlen >= sizeof(buf))
-		onreadlen = sizeof(buf) - 1;
-	ssize = read(s, buf, (size_t)onreadlen);
-	if (ssize == -1)
-		iderror(0, 0, s, errno);
-	buf[ssize] = '\0';
-	if (sscanf(buf, "%hu , %hu", &lport, &fport) != 2)
+ 	sa.sa_flags = 0;
+ 	sigemptyset(&sa.sa_mask);
+ 	sa.sa_handler = SIG_DFL;
+ 	sigaction(SIGALRM, &sa, (struct sigaction *)0);
+ 	timerclear(&(it.it_interval));
+ 	it.it_value = tv;
+ 	setitimer(ITIMER_REAL, &it, NULL);
+	size = 0;
+	bufsiz = sizeof(buf) - 1;
+ 	while (bufsiz > 0 && (size == 0 || buf[size - 1] != '\n')) {
+		FD_ZERO(&fdset);
+		FD_SET(s, &fdset);
+		if (select(s + 1, &fdset, NULL, NULL, NULL) == -1)
+			iderror(0, 0, s, errno);
+		if (ioctl(s, FIONREAD, &onreadlen) == -1)
+			iderror(0, 0, s, errno);
+		if (onreadlen > bufsiz)
+			onreadlen = bufsiz;
+		ssize = read(s, &buf[size], (size_t)onreadlen);
+		if (ssize == -1)
+			iderror(0, 0, s, errno);
+		bufsiz -= ssize;
+		size += ssize;
+ 	}
+	buf[size] = '\0';
+	/* Read two characters, and check for a delimiting character */
+	if (sscanf(buf, "%hu , %hu%c", &lport, &fport, &e) != 3 || isdigit(e))
 		iderror(0, 0, s, 0);
 	if (gflag) {
 		cp = garbage;


Index: rfc931.c
===================================================================
RCS file: /cvs/FreeBSD-CVS/src/contrib/tcp_wrappers/rfc931.c,v
retrieving revision 1.2
diff -u -r1.2 rfc931.c
--- rfc931.c	2000/02/03 10:26:59	1.2
+++ rfc931.c	2000/07/08 23:15:09
@@ -184,7 +184,7 @@
 		 * cause trouble with buggy System V stdio libraries.
 		 */
 
-		fprintf(fp, "%u,%u\r\n",
+		snprintf(buffer, sizeof(buffer), "%u,%u\r\n",
 #ifdef INET6
 			ntohs(((struct sockaddr_in *)rmt_sin)->sin_port),
 			ntohs(((struct sockaddr_in *)our_sin)->sin_port));
@@ -192,6 +192,7 @@
 			ntohs(rmt_sin->sin_port),
 			ntohs(our_sin->sin_port));
 #endif
+		fwrite(buffer, strlen(buffer), 1, fp);
 		fflush(fp);
 
 		/*

Comment 6 dwmalone 2000-07-18 17:41:23 UTC

State Changed
From-To: open->closed

Varients of patches applied to HEAD and RELENG_4.