[libsoup] soup-headers: misc improvements

From: Dan Winship <danw src gnome org>
To: commits-list gnome org
Cc:
Subject: [libsoup] soup-headers: misc improvements
Date: Wed, 21 Dec 2011 16:58:56 +0000 (UTC)
commit 22d6f350a34fc423220c265cfa98333ba4130bf0
Author: Dan Winship <danw gnome org>
Date:   Wed Dec 21 11:50:41 2011 -0500

    soup-headers: misc improvements
    
    Return an error if the headers start with '\0', ignore header lines
    with zero-length header names, and convert excess CRs to spaces. All
    of these could possibly occur in received headers, and were previously
    hitting g_return_if_fails().
    
    Add a bunch of test cases to header-tests to test these cases (and
    some others that we already handled correctly but weren't testing).
    
    Fix the documentation of @str on soup_headers_parse* to reflect
    reality.
    
    Based on a patch from Simon McVittie.
    https://bugzilla.gnome.org/show_bug.cgi?id=666316

 libsoup/soup-headers.c |   42 ++++++++++-------
 tests/header-parsing.c |  123 ++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 143 insertions(+), 22 deletions(-)
---
diff --git a/libsoup/soup-headers.c b/libsoup/soup-headers.c
index 58d53a6..965f9da 100644
--- a/libsoup/soup-headers.c
+++ b/libsoup/soup-headers.c
@@ -17,8 +17,8 @@
 /**
  * soup_headers_parse:
  * @str: the header string (including the Request-Line or Status-Line,
- * and the trailing blank line)
- * @len: length of @str up to (but not including) the terminating blank line.
+ *   but not the trailing blank line)
+ * @len: length of @str
  * @dest: #SoupMessageHeaders to store the header values in
  *
  * Parses the headers of an HTTP request or response in @str and
@@ -37,15 +37,14 @@ soup_headers_parse (const char *str, int len, SoupMessageHeaders *dest)
 {
 	const char *headers_start;
 	char *headers_copy, *name, *name_end, *value, *value_end;
-	char *eol, *sol;
+	char *eol, *sol, *p;
 	gboolean success = FALSE;
 
 	g_return_val_if_fail (str != NULL, FALSE);
 	g_return_val_if_fail (dest != NULL, FALSE);
 
-	/* Technically, the grammar does allow NUL bytes in the
-	 * headers, but this is probably a bug, and if it's not, we
-	 * can't deal with them anyway.
+	/* RFC 2616 does allow NUL bytes in the headers, but httpbis
+	 * is changing that, and we can't deal with them anyway.
 	 */
 	if (memchr (str, '\0', len))
 		return FALSE;
@@ -70,11 +69,16 @@ soup_headers_parse (const char *str, int len, SoupMessageHeaders *dest)
 	while (*(value_end + 1)) {
 		name = value_end + 1;
 		name_end = strchr (name, ':');
-		if (!name_end || name + strcspn (name, " \t\r\n") < name_end) {
-			/* Bad header; just ignore this line. Note
-			 * that if it has continuation lines, we'll
-			 * end up ignoring them too since they'll
-			 * start with spaces.
+
+		/* Reject if there is no ':', or the header name is
+		 * empty, or it contains whitespace.
+		 */
+		if (!name_end ||
+		    name_end == name ||
+		    name + strcspn (name, " \t\r\n") < name_end) {
+			/* Ignore this line. Note that if it has
+			 * continuation lines, we'll end up ignoring
+			 * them too since they'll start with spaces.
 			 */
 			value_end = strchr (name, '\n');
 			if (!value_end)
@@ -127,6 +131,10 @@ soup_headers_parse (const char *str, int len, SoupMessageHeaders *dest)
 			eol--;
 		*eol = '\0';
 
+		/* convert (illegal) '\r's to spaces */
+		for (p = strchr (value, '\r'); p; p = strchr (p, '\r'))
+			*p = ' ';
+
 		soup_message_headers_append (dest, name, value);
         }
 	success = TRUE;
@@ -138,8 +146,8 @@ done:
 
 /**
  * soup_headers_parse_request:
- * @str: the header string (including the trailing blank line)
- * @len: length of @str up to (but not including) the terminating blank line.
+ * @str: the headers (up to, but not including, the trailing blank line)
+ * @len: length of @str
  * @req_headers: #SoupMessageHeaders to store the header values in
  * @req_method: (out) (allow-none): if non-%NULL, will be filled in with the
  * request method
@@ -169,7 +177,7 @@ soup_headers_parse_request (const char          *str,
 	unsigned long major_version, minor_version;
 	char *p;
 
-	g_return_val_if_fail (str && *str, SOUP_STATUS_MALFORMED);
+	g_return_val_if_fail (str != NULL, SOUP_STATUS_MALFORMED);
 
 	/* RFC 2616 4.1 "servers SHOULD ignore any empty line(s)
 	 * received where a Request-Line is expected."
@@ -325,8 +333,8 @@ soup_headers_parse_status_line (const char       *status_line,
 
 /**
  * soup_headers_parse_response:
- * @str: the header string (including the trailing blank line)
- * @len: length of @str up to (but not including) the terminating blank line.
+ * @str: the headers (up to, but not including, the trailing blank line)
+ * @len: length of @str
  * @headers: #SoupMessageHeaders to store the header values in
  * @ver: (out) (allow-none): if non-%NULL, will be filled in with the HTTP
  * version
@@ -352,7 +360,7 @@ soup_headers_parse_response (const char          *str,
 {
 	SoupHTTPVersion version;
 
-	g_return_val_if_fail (str && *str, FALSE);
+	g_return_val_if_fail (str != NULL, FALSE);
 
 	/* Workaround for broken servers that send extra line breaks
 	 * after a response, which we then see prepended to the next
diff --git a/tests/header-parsing.c b/tests/header-parsing.c
index 63a29bb..626b27c 100644
--- a/tests/header-parsing.c
+++ b/tests/header-parsing.c
@@ -19,7 +19,7 @@ static struct RequestTest {
 	guint status;
 	const char *method, *path;
 	SoupHTTPVersion version;
-	Header headers[4];
+	Header headers[10];
 } reqtests[] = {
 	/**********************/
 	/*** VALID REQUESTS ***/
@@ -206,7 +206,7 @@ static struct RequestTest {
 	/* RFC 2616 section 19.3 says we SHOULD accept these */
 
 	{ "LF instead of CRLF after header",
-	  "GET / HTTP/1.1\nHost: example.com\nConnection: close\n", -1,
+	  "GET / HTTP/1.1\r\nHost: example.com\nConnection: close\n", -1,
 	  SOUP_STATUS_OK,
 	  "GET", "/", SOUP_HTTP_1_1,
 	  { { "Host", "example.com" },
@@ -224,6 +224,18 @@ static struct RequestTest {
 	  }
 	},
 
+	{ "Mixed CRLF/LF",
+	  "GET / HTTP/1.1\r\na: b\r\nc: d\ne: f\r\ng: h\n", -1,
+	  SOUP_STATUS_OK,
+	  "GET", "/", SOUP_HTTP_1_1,
+	  { { "a", "b" },
+	    { "c", "d" },
+	    { "e", "f" },
+	    { "g", "h" },
+	    { NULL }
+	  }
+	},
+
 	{ "Req w/ incorrect whitespace in Request-Line",
 	  "GET  /\tHTTP/1.1\r\nHost: example.com\r\n", -1,
 	  SOUP_STATUS_OK,
@@ -242,7 +254,11 @@ static struct RequestTest {
 	  }
 	},
 
-	/* qv bug 579318, do_bad_header_tests() below */
+	/* If the request/status line is parseable, then we
+	 * just ignore any invalid-looking headers after that.
+	 * (qv bug 579318).
+	 */
+
 	{ "Req w/ mangled header",
 	  "GET / HTTP/1.1\r\nHost: example.com\r\nFoo one\r\nBar: two\r\n", -1,
 	  SOUP_STATUS_OK,
@@ -253,6 +269,77 @@ static struct RequestTest {
 	  }
 	},
 
+	{ "First header line is continuation",
+	  "GET / HTTP/1.1\r\n b\r\nHost: example.com\r\nc: d\r\n", -1,
+	  SOUP_STATUS_OK,
+	  "GET", "/", SOUP_HTTP_1_1,
+	  { { "Host", "example.com" },
+	    { "c", "d" },
+	    { NULL }
+	  }
+	},
+
+	{ "Zero-length header name",
+	  "GET / HTTP/1.1\r\na: b\r\n: example.com\r\nc: d\r\n", -1,
+	  SOUP_STATUS_OK,
+	  "GET", "/", SOUP_HTTP_1_1,
+	  { { "a", "b" },
+	    { "c", "d" },
+	    { NULL }
+	  }
+	},
+
+	{ "CR in header name",
+	  "GET / HTTP/1.1\r\na: b\r\na\rb: cd\r\nx\r: y\r\n\rz: w\r\nc: d\r\n", -1,
+	  SOUP_STATUS_OK,
+	  "GET", "/", SOUP_HTTP_1_1,
+	  { { "a", "b" },
+	    { "c", "d" },
+	    { NULL }
+	  }
+	},
+
+	{ "CR in header value",
+	  "GET / HTTP/1.1\r\na: b\r\nHost: example\rcom\r\np: \rq\r\ns: t\r\r\nc: d\r\n", -1,
+	  SOUP_STATUS_OK,
+	  "GET", "/", SOUP_HTTP_1_1,
+	  { { "a", "b" },
+	    { "Host", "example com" },	/* CR in the middle turns to space */
+	    { "p", "q" },		/* CR at beginning is ignored */
+	    { "s", "t" },		/* CR at end is ignored */
+	    { "c", "d" },
+	    { NULL }
+	  }
+	},
+
+	{ "Tab in header name",
+	  "GET / HTTP/1.1\r\na: b\r\na\tb: cd\r\nx\t: y\r\np: q\r\n\tz: w\r\nc: d\r\n", -1,
+	  SOUP_STATUS_OK,
+	  "GET", "/", SOUP_HTTP_1_1,
+	  { { "a", "b" },
+	    /* Tab anywhere in the header name causes it to be
+	     * ignored... except at beginning of line where it's a
+	     * continuation line
+	     */
+	    { "p", "q z: w" },
+	    { "c", "d" },
+	    { NULL }
+	  }
+	},
+
+	{ "Tab in header value",
+	  "GET / HTTP/1.1\r\na: b\r\nab: c\td\r\nx: \ty\r\nz: w\t\r\nc: d\r\n", -1,
+	  SOUP_STATUS_OK,
+	  "GET", "/", SOUP_HTTP_1_1,
+	  { { "a", "b" },
+	    { "ab", "c\td" },	/* internal tab preserved */
+	    { "x", "y" },	/* leading tab ignored */
+	    { "z", "w" },	/* trailing tab ignored */
+	    { "c", "d" },
+	    { NULL }
+	  }
+	},
+
 	/************************/
 	/*** INVALID REQUESTS ***/
 	/************************/
@@ -299,6 +386,13 @@ static struct RequestTest {
 	  { { NULL } }
 	},
 
+	{ "NUL at beginning of Method",
+	  "\x00 / HTTP/1.1\r\nHost: example.com\r\n", 35,
+	  SOUP_STATUS_BAD_REQUEST,
+	  NULL, NULL, -1,
+	  { { NULL } }
+	},
+
 	{ "NUL in Path",
 	  "GET /\x00 HTTP/1.1\r\nHost: example.com\r\n", 38,
 	  SOUP_STATUS_BAD_REQUEST,
@@ -306,7 +400,14 @@ static struct RequestTest {
 	  { { NULL } }
 	},
 
-	{ "NUL in Header",
+	{ "NUL in header name",
+	  "GET / HTTP/1.1\r\n\x00: silly\r\n", 37,
+	  SOUP_STATUS_BAD_REQUEST,
+	  NULL, NULL, -1,
+	  { { NULL } }
+	},
+
+	{ "NUL in header value",
 	  "GET / HTTP/1.1\r\nHost: example\x00com\r\n", 37,
 	  SOUP_STATUS_BAD_REQUEST,
 	  NULL, NULL, -1,
@@ -535,13 +636,25 @@ static struct ResponseTest {
 	  { { NULL } }
 	},
 
+	{ "NUL at start",
+	  "\x00HTTP/1.1 200 OK\r\nFoo: bar\r\n", 28,
+	  -1, 0, NULL,
+	  { { NULL } }
+	},
+
 	{ "NUL in Reason Phrase",
 	  "HTTP/1.1 200 O\x00K\r\nFoo: bar\r\n", 28,
 	  -1, 0, NULL,
 	  { { NULL } }
 	},
 
-	{ "NUL in Header",
+	{ "NUL in header name",
+	  "HTTP/1.1 200 OK\r\nF\x00oo: bar\r\n", 28,
+	  -1, 0, NULL,
+	  { { NULL } }
+	},
+
+	{ "NUL in header value",
 	  "HTTP/1.1 200 OK\r\nFoo: b\x00ar\r\n", 28,
 	  -1, 0, NULL,
 	  { { NULL } }
[Date Prev][Date Next] [Thread Prev][Thread Next] [Thread Index] [Date Index] [Author Index]