Must really be safe_read(), not full_read()? (was: [PATCH] fix httpd lockup in cgi POSTs)
Matthias Reichl
hias at horus.com
Tue Feb 13 00:42:51 UTC 2007
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)) */
More information about the busybox
mailing list