httpd: don't send error body with HEAD method

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

httpd: don't send error body with HEAD method

Bertrand Janin-2
Hi,

This patch updates server_abort_http() to only send the body of default http
error if the method is not HEAD. I first noticed that with curl -v -I which
complains about the excess data:

    * Excess found in a non pipelined read: excess = 397 url = /asd (zero-length body)


Index: usr.sbin/httpd/server_http.c
===================================================================
RCS file: /cvs/src/usr.sbin/httpd/server_http.c,v
retrieving revision 1.54
diff -u -p -r1.54 server_http.c
--- usr.sbin/httpd/server_http.c 25 Oct 2014 03:23:49 -0000 1.54
+++ usr.sbin/httpd/server_http.c 23 Nov 2014 23:49:44 -0000
@@ -665,8 +665,9 @@ server_abort_http(struct client *clt, u_
  struct server *srv = clt->clt_srv;
  struct server_config *srv_conf = &srv->srv_conf;
  struct bufferevent *bev = clt->clt_bev;
+ struct http_descriptor *desc = clt->clt_descreq;
  const char *httperr = NULL, *text = "";
- char *httpmsg, *extraheader = NULL;
+ char *httpmsg, *body = NULL, *extraheader = NULL;
  char tmbuf[32], hbuf[128];
  const char *style;
 
@@ -706,10 +707,33 @@ server_abort_http(struct client *clt, u_
  break;
  }
 
- /* A CSS stylesheet allows minimal customization by the user */
- style = "body { background-color: white; color: black; font-family: "
-    "'Comic Sans MS', 'Chalkboard SE', 'Comic Neue', sans-serif; }\n"
-    "hr { border: 0; border-bottom: 1px dashed; }\n";
+ /* Generate the body (not needed for HEAD requests) */
+ if (desc->http_method != HTTP_METHOD_HEAD) {
+ /* A CSS stylesheet allows minimal customization by the user */
+ style = "body { background-color: white; color: black; "
+ "font-family: 'Comic Sans MS', 'Chalkboard SE', "
+ "'Comic Neue', sans-serif; }\n"
+ "hr { border: 0; border-bottom: 1px dashed; }\n";
+
+ if (asprintf(&body,
+    "<!DOCTYPE HTML PUBLIC "
+    "\"-//W3C//DTD HTML 4.01 Transitional//EN\">\n"
+    "<html>\n"
+    "<head>\n"
+    "<title>%03d %s</title>\n"
+    "<style type=\"text/css\"><!--\n%s\n--></style>\n"
+    "</head>\n"
+    "<body>\n"
+    "<h1>%03d %s</h1>\n"
+    "<div id='m'>%s</div>\n"
+    "<hr>\n<address>%s</address>\n"
+    "</body>\n"
+    "</html>\n",
+    code, httperr, style, code, httperr, text,
+    HTTPD_SERVERNAME) == -1)
+ goto done;
+ }
+
  /* Generate simple HTTP+HTML error document */
  if (asprintf(&httpmsg,
     "HTTP/1.0 %03d %s\r\n"
@@ -719,28 +743,16 @@ server_abort_http(struct client *clt, u_
     "Content-Type: text/html\r\n"
     "%s"
     "\r\n"
-    "<!DOCTYPE HTML PUBLIC "
-    "\"-//W3C//DTD HTML 4.01 Transitional//EN\">\n"
-    "<html>\n"
-    "<head>\n"
-    "<title>%03d %s</title>\n"
-    "<style type=\"text/css\"><!--\n%s\n--></style>\n"
-    "</head>\n"
-    "<body>\n"
-    "<h1>%03d %s</h1>\n"
-    "<div id='m'>%s</div>\n"
-    "<hr>\n<address>%s</address>\n"
-    "</body>\n"
-    "</html>\n",
+    "%s",
     code, httperr, tmbuf, HTTPD_SERVERNAME,
     extraheader == NULL ? "" : extraheader,
-    code, httperr, style, code, httperr, text,
-    HTTPD_SERVERNAME) == -1)
+    body == NULL ? "" : body) == -1)
  goto done;
 
  /* Dump the message without checking for success */
  server_dump(clt, httpmsg, strlen(httpmsg));
  free(httpmsg);
+ free(body);
 
  done:
  free(extraheader);

Reply | Threaded
Open this post in threaded view
|

Re: httpd: don't send error body with HEAD method

Florian Obser-2
On Sun, Nov 23, 2014 at 08:15:47PM -0500, Bertrand Janin wrote:
> Hi,
>
> This patch updates server_abort_http() to only send the body of default http
> error if the method is not HEAD. I first noticed that with curl -v -I which
> complains about the excess data:
>
>     * Excess found in a non pipelined read: excess = 397 url = /asd (zero-length body)

Nice catch. However, your diff will leak body if the second
asprintf(3) fails.
Since we are probably not supposed to send a "Content-Type" header I
think it makes sense to duplicate the httpmsg generating code in this
case; it feels easier.
While in there nuke unneeded text variable and shuffle variable
declarations around.

OK?

diff --git server_http.c server_http.c
index 05e3381..bc77fc4 100644
--- server_http.c
+++ server_http.c
@@ -665,10 +665,10 @@ server_abort_http(struct client *clt, u_int code, const char *msg)
  struct server *srv = clt->clt_srv;
  struct server_config *srv_conf = &srv->srv_conf;
  struct bufferevent *bev = clt->clt_bev;
- const char *httperr = NULL, *text = "";
+ struct http_descriptor *desc = clt->clt_descreq;
+ const char *httperr, *style;
  char *httpmsg, *extraheader = NULL;
  char tmbuf[32], hbuf[128];
- const char *style;
 
  if ((httperr = server_httperror_byid(code)) == NULL)
  httperr = "Unknown Error";
@@ -706,37 +706,48 @@ server_abort_http(struct client *clt, u_int code, const char *msg)
  break;
  }
 
- /* A CSS stylesheet allows minimal customization by the user */
- style = "body { background-color: white; color: black; font-family: "
-    "'Comic Sans MS', 'Chalkboard SE', 'Comic Neue', sans-serif; }\n"
-    "hr { border: 0; border-bottom: 1px dashed; }\n";
- /* Generate simple HTTP+HTML error document */
- if (asprintf(&httpmsg,
-    "HTTP/1.0 %03d %s\r\n"
-    "Date: %s\r\n"
-    "Server: %s\r\n"
-    "Connection: close\r\n"
-    "Content-Type: text/html\r\n"
-    "%s"
-    "\r\n"
-    "<!DOCTYPE HTML PUBLIC "
-    "\"-//W3C//DTD HTML 4.01 Transitional//EN\">\n"
-    "<html>\n"
-    "<head>\n"
-    "<title>%03d %s</title>\n"
-    "<style type=\"text/css\"><!--\n%s\n--></style>\n"
-    "</head>\n"
-    "<body>\n"
-    "<h1>%03d %s</h1>\n"
-    "<div id='m'>%s</div>\n"
-    "<hr>\n<address>%s</address>\n"
-    "</body>\n"
-    "</html>\n",
-    code, httperr, tmbuf, HTTPD_SERVERNAME,
-    extraheader == NULL ? "" : extraheader,
-    code, httperr, style, code, httperr, text,
-    HTTPD_SERVERNAME) == -1)
- goto done;
+ if (desc->http_method == HTTP_METHOD_HEAD) {
+ /* Generate HTTP header */
+ if (asprintf(&httpmsg,
+    "HTTP/1.0 %03d %s\r\n"
+    "Date: %s\r\n"
+    "Server: %s\r\n"
+    "Connection: close\r\n"
+    "%s\r\n", code, httperr, tmbuf, HTTPD_SERVERNAME,
+    extraheader == NULL ? "" : extraheader) == -1)
+ goto done;
+ } else {
+ /* A CSS stylesheet allows minimal customization by the user */
+ style = "body { background-color: white; color: black; "
+    "font-family: 'Comic Sans MS', 'Chalkboard SE', "
+    "'Comic Neue', sans-serif; }\n"
+    "hr { border: 0; border-bottom: 1px dashed; }\n";
+ /* Generate simple HTTP+HTML error document */
+ if (asprintf(&httpmsg,
+    "HTTP/1.0 %03d %s\r\n"
+    "Date: %s\r\n"
+    "Server: %s\r\n"
+    "Connection: close\r\n"
+    "Content-Type: text/html\r\n"
+    "%s"
+    "\r\n"
+    "<!DOCTYPE HTML PUBLIC "
+    "\"-//W3C//DTD HTML 4.01 Transitional//EN\">\n"
+    "<html>\n"
+    "<head>\n"
+    "<title>%03d %s</title>\n"
+    "<style type=\"text/css\"><!--\n%s\n--></style>\n"
+    "</head>\n"
+    "<body>\n"
+    "<h1>%03d %s</h1>\n"
+    "<hr>\n<address>%s</address>\n"
+    "</body>\n"
+    "</html>\n",
+    code, httperr, tmbuf, HTTPD_SERVERNAME,
+    extraheader == NULL ? "" : extraheader, code, httperr,
+    style, code, httperr, HTTPD_SERVERNAME) == -1)
+ goto done;
+ }
 
  /* Dump the message without checking for success */
  server_dump(clt, httpmsg, strlen(httpmsg));


--
I'm not entirely sure you are real.

Reply | Threaded
Open this post in threaded view
|

Re: httpd: don't send error body with HEAD method

Bertrand Janin-2
Florian Obser wrote :

> On Sun, Nov 23, 2014 at 08:15:47PM -0500, Bertrand Janin wrote:
> > Hi,
> >
> > This patch updates server_abort_http() to only send the body of default http
> > error if the method is not HEAD. I first noticed that with curl -v -I which
> > complains about the excess data:
> >
> >     * Excess found in a non pipelined read: excess = 397 url = /asd (zero-length body)
>
> Nice catch. However, your diff will leak body if the second
> asprintf(3) fails.
> Since we are probably not supposed to send a "Content-Type" header I
> think it makes sense to duplicate the httpmsg generating code in this
> case; it feels easier.
> While in there nuke unneeded text variable and shuffle variable
> declarations around.

Indeed, that's much better.

-b

Reply | Threaded
Open this post in threaded view
|

Re: httpd: don't send error body with HEAD method

Philip Guenther-2
In reply to this post by Florian Obser-2
On Mon, Nov 24, 2014 at 11:24 AM, Florian Obser <[hidden email]> wrote:
...
> Since we are probably not supposed to send a "Content-Type" header I
> think it makes sense to duplicate the httpmsg generating code in this
> case;

If a GET of that resource would have a Content-Type, then the HEAD of
it should have one.  Per RFC 2616, HEAD and GET should return the same
header fields and only differ by HEAD leaving out the body.


Philip

Reply | Threaded
Open this post in threaded view
|

Re: httpd: don't send error body with HEAD method

Bertrand Janin-2
Philip Guenther wrote :
> On Mon, Nov 24, 2014 at 11:24 AM, Florian Obser <[hidden email]> wrote:
> ...
> > Since we are probably not supposed to send a "Content-Type" header I
> > think it makes sense to duplicate the httpmsg generating code in this
> > case;
>
> If a GET of that resource would have a Content-Type, then the HEAD of
> it should have one.  Per RFC 2616, HEAD and GET should return the same
> header fields and only differ by HEAD leaving out the body.

Here is an updated patch which fix the leak identified by Florian, ensures the
headers are identical with HEAD and non-HEAD methods and adds Content-Length.


Index: usr.sbin/httpd/server_http.c
===================================================================
RCS file: /cvs/src/usr.sbin/httpd/server_http.c,v
retrieving revision 1.54
diff -u -p -r1.54 server_http.c
--- usr.sbin/httpd/server_http.c 25 Oct 2014 03:23:49 -0000 1.54
+++ usr.sbin/httpd/server_http.c 25 Nov 2014 00:14:15 -0000
@@ -665,10 +665,10 @@ server_abort_http(struct client *clt, u_
  struct server *srv = clt->clt_srv;
  struct server_config *srv_conf = &srv->srv_conf;
  struct bufferevent *bev = clt->clt_bev;
- const char *httperr = NULL, *text = "";
- char *httpmsg, *extraheader = NULL;
+ struct http_descriptor *desc = clt->clt_descreq;
+ const char *httperr = NULL, *style;
+ char *httpmsg, *body = NULL, *extraheader = NULL;
  char tmbuf[32], hbuf[128];
- const char *style;
 
  if ((httperr = server_httperror_byid(code)) == NULL)
  httperr = "Unknown Error";
@@ -710,15 +710,9 @@ server_abort_http(struct client *clt, u_
  style = "body { background-color: white; color: black; font-family: "
     "'Comic Sans MS', 'Chalkboard SE', 'Comic Neue', sans-serif; }\n"
     "hr { border: 0; border-bottom: 1px dashed; }\n";
- /* Generate simple HTTP+HTML error document */
- if (asprintf(&httpmsg,
-    "HTTP/1.0 %03d %s\r\n"
-    "Date: %s\r\n"
-    "Server: %s\r\n"
-    "Connection: close\r\n"
-    "Content-Type: text/html\r\n"
-    "%s"
-    "\r\n"
+
+ /* Generate simple HTML error document */
+ if (asprintf(&body,
     "<!DOCTYPE HTML PUBLIC "
     "\"-//W3C//DTD HTML 4.01 Transitional//EN\">\n"
     "<html>\n"
@@ -728,14 +722,26 @@ server_abort_http(struct client *clt, u_
     "</head>\n"
     "<body>\n"
     "<h1>%03d %s</h1>\n"
-    "<div id='m'>%s</div>\n"
     "<hr>\n<address>%s</address>\n"
     "</body>\n"
     "</html>\n",
-    code, httperr, tmbuf, HTTPD_SERVERNAME,
+    code, httperr, style, code, httperr, HTTPD_SERVERNAME) == -1)
+ goto done;
+
+ /* Add basic HTTP headers */
+ if (asprintf(&httpmsg,
+    "HTTP/1.0 %03d %s\r\n"
+    "Date: %s\r\n"
+    "Server: %s\r\n"
+    "Connection: close\r\n"
+    "Content-Type: text/html\r\n"
+    "Content-Length: %ld\r\n"
+    "%s"
+    "\r\n"
+    "%s",
+    code, httperr, tmbuf, HTTPD_SERVERNAME, strlen(body),
     extraheader == NULL ? "" : extraheader,
-    code, httperr, style, code, httperr, text,
-    HTTPD_SERVERNAME) == -1)
+    desc->http_method == HTTP_METHOD_HEAD ? "" : body) == -1)
  goto done;
 
  /* Dump the message without checking for success */
@@ -743,6 +749,7 @@ server_abort_http(struct client *clt, u_
  free(httpmsg);
 
  done:
+ free(body);
  free(extraheader);
  if (asprintf(&httpmsg, "%s (%03d %s)", msg, code, httperr) == -1) {
  server_close(clt, msg);

Reply | Threaded
Open this post in threaded view
|

Re: httpd: don't send error body with HEAD method

Bertrand Janin-2
Bertrand Janin wrote :

> Philip Guenther wrote :
> > On Mon, Nov 24, 2014 at 11:24 AM, Florian Obser <[hidden email]> wrote:
> > ...
> > > Since we are probably not supposed to send a "Content-Type" header I
> > > think it makes sense to duplicate the httpmsg generating code in this
> > > case;
> >
> > If a GET of that resource would have a Content-Type, then the HEAD of
> > it should have one.  Per RFC 2616, HEAD and GET should return the same
> > header fields and only differ by HEAD leaving out the body.
>
> Here is an updated patch which fix the leak identified by Florian, ensures the
> headers are identical with HEAD and non-HEAD methods and adds Content-Length.

Replaced strlen() with the output from asprintf(), any comments?


Index: usr.sbin/httpd/server_http.c
===================================================================
RCS file: /cvs/src/usr.sbin/httpd/server_http.c,v
retrieving revision 1.54
diff -u -p -r1.54 server_http.c
--- usr.sbin/httpd/server_http.c 25 Oct 2014 03:23:49 -0000 1.54
+++ usr.sbin/httpd/server_http.c 26 Nov 2014 03:05:48 -0000
@@ -665,10 +665,11 @@ server_abort_http(struct client *clt, u_
  struct server *srv = clt->clt_srv;
  struct server_config *srv_conf = &srv->srv_conf;
  struct bufferevent *bev = clt->clt_bev;
- const char *httperr = NULL, *text = "";
- char *httpmsg, *extraheader = NULL;
+ struct http_descriptor *desc = clt->clt_descreq;
+ const char *httperr = NULL, *style;
+ char *httpmsg, *body = NULL, *extraheader = NULL;
  char tmbuf[32], hbuf[128];
- const char *style;
+ int bodylen;
 
  if ((httperr = server_httperror_byid(code)) == NULL)
  httperr = "Unknown Error";
@@ -710,15 +711,9 @@ server_abort_http(struct client *clt, u_
  style = "body { background-color: white; color: black; font-family: "
     "'Comic Sans MS', 'Chalkboard SE', 'Comic Neue', sans-serif; }\n"
     "hr { border: 0; border-bottom: 1px dashed; }\n";
- /* Generate simple HTTP+HTML error document */
- if (asprintf(&httpmsg,
-    "HTTP/1.0 %03d %s\r\n"
-    "Date: %s\r\n"
-    "Server: %s\r\n"
-    "Connection: close\r\n"
-    "Content-Type: text/html\r\n"
-    "%s"
-    "\r\n"
+
+ /* Generate simple HTML error document */
+ if ((bodylen = asprintf(&body,
     "<!DOCTYPE HTML PUBLIC "
     "\"-//W3C//DTD HTML 4.01 Transitional//EN\">\n"
     "<html>\n"
@@ -728,14 +723,26 @@ server_abort_http(struct client *clt, u_
     "</head>\n"
     "<body>\n"
     "<h1>%03d %s</h1>\n"
-    "<div id='m'>%s</div>\n"
     "<hr>\n<address>%s</address>\n"
     "</body>\n"
     "</html>\n",
-    code, httperr, tmbuf, HTTPD_SERVERNAME,
+    code, httperr, style, code, httperr, HTTPD_SERVERNAME)) == -1)
+ goto done;
+
+ /* Add basic HTTP headers */
+ if (asprintf(&httpmsg,
+    "HTTP/1.0 %03d %s\r\n"
+    "Date: %s\r\n"
+    "Server: %s\r\n"
+    "Connection: close\r\n"
+    "Content-Type: text/html\r\n"
+    "Content-Length: %d\r\n"
+    "%s"
+    "\r\n"
+    "%s",
+    code, httperr, tmbuf, HTTPD_SERVERNAME, bodylen,
     extraheader == NULL ? "" : extraheader,
-    code, httperr, style, code, httperr, text,
-    HTTPD_SERVERNAME) == -1)
+    desc->http_method == HTTP_METHOD_HEAD ? "" : body) == -1)
  goto done;
 
  /* Dump the message without checking for success */
@@ -743,6 +750,7 @@ server_abort_http(struct client *clt, u_
  free(httpmsg);
 
  done:
+ free(body);
  free(extraheader);
  if (asprintf(&httpmsg, "%s (%03d %s)", msg, code, httperr) == -1) {
  server_close(clt, msg);