Must really be safe_read(), not full_read()? (was: [PATCH] fix httpd lockup in cgi POSTs)

[Matthias Reichl]

On Mon, Feb 12, 2007 at 11:34:03PM +0100, Denis Vlasenko wrote:
> On Monday 12 February 2007 23:21, Matthias Reichl wrote:
> > Would be OK for me, too. OTOH: it's a simple patch and it assures
> > that the current httpd could also deal with insane cgis.
> 
> NO, IT CAN'T.
> 
> If cgi will output "HTT" and then block in read() from fd#0
> while httpd is also blocked in full_read() trying to get at least
> four bytes, we will deadlock.

OK, you are right. We have to avoid deadlock situations,

Here's a new patch. It delays the header check against "HTTP" until
enough bytes are read from the CGI. All this is done non-blocking.
In case of a premature EOF (eg CGI only writing "hi\n") a
"HTTP/1.0 200 OK" plus the data is sent. The rest of the code is
as before.

I hope this fixes the issues (at the moment :-)

so long,

Hias
-------------- next part --------------
--- busybox-svn.orig/networking/httpd.c	2007-02-11 23:29:09.000000000 +0100
+++ busybox-svn/networking/httpd.c	2007-02-13 01:36:58.000000000 +0100
@@ -1137,6 +1137,14 @@
 		fd_set readSet;
 		fd_set writeSet;
 		char wbuf[128];
+		/* default response if not set by cgi */
+		static const char HTTP_200[] = "HTTP/1.0 200 OK\r\n\r\n";
+		/* only check the first 4 bytes of cgi response (for "HTTP") */
+#define HTTP_200_CHECK_LEN 4
+#if HTTP_200_CHECK_LEN >= MAX_MEMORY_BUFF
+# error "HTTP_200_CHECK_LEN >= MAX_MEMORY_BUFF"
+#endif
+		int fcount = 0; /* byte counter for first line */
 		int nfound;
 		int count;
 
@@ -1206,41 +1214,59 @@
 			/* There is something to read from CGI */
 			int s = config->accepted_socket;
 			char *rbuf = config->buf;
+			if (firstLine) {
+				/* try to read the first bytes of the CGI response so we can
+				 * check if it contained a HTTP/x.y ... response
+				 * note: do it non-blocking, otherwise httpd might deadlock! */
+				count = safe_read(inFd, rbuf + fcount, HTTP_200_CHECK_LEN - fcount);
+				if (count == 0) {
+					/* eof and we couldn't check for a valid HTTP/... header,
+					 * so add one and write out the received data */
+					if (fcount) {
+							full_write(s, HTTP_200, sizeof(HTTP_200)-1);
+							full_write(s, rbuf, fcount);
+					}
+					break;  /* closed */
+				}
+				if (count < 0)
+					continue; /* huh, error, why continue?? */
+
+				fcount += count;
+
+				if (fcount == HTTP_200_CHECK_LEN) {
+					rbuf[fcount] = '\0';
+					/* check to see if the user script added headers */
+					/* (NB: buggy wrt cgi splitting "HTTP OK") */
+					if (memcmp(rbuf, HTTP_200, HTTP_200_CHECK_LEN) != 0) {
+						/* there is no "HTTP", do it ourself */
+						if (full_write(s, HTTP_200, sizeof(HTTP_200)-1) != sizeof(HTTP_200)-1)
+							break;
+					}
+					/* Example of valid GCI without "Content-type:"
+					 * echo -en "HTTP/1.0 302 Found\r\n"
+					 * echo -en "Location: http://www.busybox.net\r\n"
+					 * echo -en "\r\n"
+					if (!strstr(rbuf, "ontent-")) {
+						full_write(s, "Content-type: text/plain\r\n\r\n", 28);
+					}
+					 */
+					if (full_write(s, rbuf, fcount) != fcount)
+						break;
+					firstLine = 0;
+				}
+			} else {
 #define PIPESIZE PIPE_BUF
 #if PIPESIZE >= MAX_MEMORY_BUFF
 # error "PIPESIZE >= MAX_MEMORY_BUFF"
 #endif
-			/* Must use safe_read, not full_read, because
-			 * cgi may output a few first lines and then wait
-			 * for POSTDATA without closing stdout.
-			 * With full_read we may wait here forever. */
-			count = safe_read(inFd, rbuf, PIPESIZE);
-			if (count == 0)
-				break;  /* closed */
-			if (count < 0)
-				continue; /* huh, error, why continue?? */
-
-			if (firstLine) {
-				static const char HTTP_200[] = "HTTP/1.0 200 OK\r\n\r\n";
-				rbuf[count] = '\0';
-				/* check to see if the user script added headers */
-				/* (NB: buggy wrt cgi splitting "HTTP OK") */
-				if (memcmp(rbuf, HTTP_200, 4) != 0) {
-					/* there is no "HTTP", do it ourself */
-					full_write(s, HTTP_200, sizeof(HTTP_200)-1);
-				}
-				/* Example of valid GCI without "Content-type:"
-				 * echo -en "HTTP/1.0 302 Found\r\n"
-				 * echo -en "Location: http://www.busybox.net\r\n"
-				 * echo -en "\r\n"
-				if (!strstr(rbuf, "ontent-")) {
-					full_write(s, "Content-type: text/plain\r\n\r\n", 28);
-				}
-				 */
-				firstLine = 0;
+				count = safe_read(inFd, rbuf, PIPESIZE);
+				if (count == 0)
+					break;  /* closed */
+				if (count < 0)
+					continue; /* huh, error, why continue?? */
+				if (full_write(s, rbuf, count) != count)
+					break;
 			}
-			if (full_write(s, rbuf, count) != count)
-				break;
 			if (DEBUG)
 				fprintf(stderr, "cgi read %d bytes: '%.*s'\n", count, count, rbuf);
 		} /* if (FD_ISSET(inFd)) */