From: Nick Mathewson <nickm@torproject.org>
Date: Wed, 6 Oct 2010 15:48:52 +0000 (-0400)
Subject: Let evhttp_parse_query return -1 on failure
X-Git-Tag: release-2.0.9-rc~67^2~5
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=b1756d019d23656170d0295ed05e13c2fc201ca7;p=libevent

Let evhttp_parse_query return -1 on failure

We already detected certain malformed queries, but we responded by
aborting the query-parsing process half-way through without telling
the user.  Now, if query-parsing fails, no headers are returned, and
evhttp_parse_query returns -1.
---

diff --git a/http.c b/http.c
index af1ff888..30b09e9e 100644
--- a/http.c
+++ b/http.c
@@ -2399,26 +2399,25 @@ evhttp_decode_uri(const char *uri)
  * The arguments are separated by key and value.
  */
 
-void
+int
 evhttp_parse_query(const char *uri, struct evkeyvalq *headers)
 {
 	char *line;
 	char *argument;
 	char *p;
+	int result = -1;
 
 	TAILQ_INIT(headers);
 
 	/* No arguments - we are done */
 	if (strchr(uri, '?') == NULL)
-		return;
+		return 0;
 
 	if ((line = mm_strdup(uri)) == NULL) {
-		/* TODO(niels): does this function need to return -1 */
 		event_warn("%s: strdup", __func__);
-		return;
+		return -1;
 	}
 
-
 	argument = line;
 
 	/* We already know that there has to be a ? */
@@ -2431,13 +2430,13 @@ evhttp_parse_query(const char *uri, struct evkeyvalq *headers)
 
 		value = argument;
 		key = strsep(&value, "=");
-		if (value == NULL || *key == '\0')
+		if (value == NULL || *key == '\0') {
 			goto error;
+		}
 
 		if ((decoded_value = mm_malloc(strlen(value) + 1)) == NULL) {
-			/* TODO(niels): do we need to return -1 here? */
 			event_warn("%s: mm_malloc", __func__);
-			break;
+			goto error;
 		}
 		evhttp_decode_uri_internal(value, strlen(value),
 		    decoded_value, 1 /*always_decode_plus*/);
@@ -2446,8 +2445,13 @@ evhttp_parse_query(const char *uri, struct evkeyvalq *headers)
 		mm_free(decoded_value);
 	}
 
- error:
+	result = 0;
+	goto done;
+error:
+	evhttp_clear_headers(headers);
+done:
 	mm_free(line);
+	return result;
 }
 
 static struct evhttp_cb *
diff --git a/include/event2/http.h b/include/event2/http.h
index cab068a5..10822b93 100644
--- a/include/event2/http.h
+++ b/include/event2/http.h
@@ -541,9 +541,9 @@ char *evhttp_decode_uri(const char *uri);
 
    @param uri the request URI
    @param headers the head of the evkeyval queue
+   @return 0 on success, -1 on failure
  */
-void evhttp_parse_query(const char *uri, struct evkeyvalq *headers);
-
+int evhttp_parse_query(const char *uri, struct evkeyvalq *headers);
 
 /**
  * Escape HTML character entities in a string.
diff --git a/test/regress_http.c b/test/regress_http.c
index 5b899a23..2554b7dd 100644
--- a/test/regress_http.c
+++ b/test/regress_http.c
@@ -1639,28 +1639,71 @@ static void
 http_parse_query_test(void *ptr)
 {
 	struct evkeyvalq headers;
+	int r;
 
 	TAILQ_INIT(&headers);
 
-	evhttp_parse_query("http://www.test.com/?q=test", &headers);
+	r = evhttp_parse_query("http://www.test.com/?q=test", &headers);
 	tt_want(validate_header(&headers, "q", "test") == 0);
+	tt_int_op(r, ==, 0);
 	evhttp_clear_headers(&headers);
 
-	evhttp_parse_query("http://www.test.com/?q=test&foo=bar", &headers);
+	r = evhttp_parse_query("http://www.test.com/?q=test&foo=bar", &headers);
 	tt_want(validate_header(&headers, "q", "test") == 0);
 	tt_want(validate_header(&headers, "foo", "bar") == 0);
+	tt_int_op(r, ==, 0);
 	evhttp_clear_headers(&headers);
 
-	evhttp_parse_query("http://www.test.com/?q=test+foo", &headers);
+	r = evhttp_parse_query("http://www.test.com/?q=test+foo", &headers);
 	tt_want(validate_header(&headers, "q", "test foo") == 0);
+	tt_int_op(r, ==, 0);
 	evhttp_clear_headers(&headers);
 
-	evhttp_parse_query("http://www.test.com/?q=test%0Afoo", &headers);
+	r = evhttp_parse_query("http://www.test.com/?q=test%0Afoo", &headers);
 	tt_want(validate_header(&headers, "q", "test\nfoo") == 0);
+	tt_int_op(r, ==, 0);
 	evhttp_clear_headers(&headers);
 
-	evhttp_parse_query("http://www.test.com/?q=test%0Dfoo", &headers);
+	r = evhttp_parse_query("http://www.test.com/?q=test%0Dfoo", &headers);
 	tt_want(validate_header(&headers, "q", "test\rfoo") == 0);
+	tt_int_op(r, ==, 0);
+	evhttp_clear_headers(&headers);
+
+	r = evhttp_parse_query("http://www.test.com/?q=test&&q2", &headers);
+	tt_int_op(r, ==, -1);
+	evhttp_clear_headers(&headers);
+
+	r = evhttp_parse_query("http://www.test.com/?q=test+this", &headers);
+	tt_want(validate_header(&headers, "q", "test this") == 0);
+	tt_int_op(r, ==, 0);
+	evhttp_clear_headers(&headers);
+
+	r = evhttp_parse_query("http://www.test.com/?q=test&q2=foo", &headers);
+	tt_int_op(r, ==, 0);
+	tt_want(validate_header(&headers, "q", "test") == 0);
+	tt_want(validate_header(&headers, "q2", "foo") == 0);
+	evhttp_clear_headers(&headers);
+
+	r = evhttp_parse_query("http://www.test.com/?q&q2=foo", &headers);
+	tt_int_op(r, ==, -1);
+	evhttp_clear_headers(&headers);
+
+	r = evhttp_parse_query("http://www.test.com/?q=foo&q2", &headers);
+	tt_int_op(r, ==, -1);
+	evhttp_clear_headers(&headers);
+
+	r = evhttp_parse_query("http://www.test.com/?q=foo&q2&q3=x", &headers);
+	tt_int_op(r, ==, -1);
+	evhttp_clear_headers(&headers);
+
+	r = evhttp_parse_query("http://www.test.com/?q=&q2=&q3=", &headers);
+	tt_int_op(r, ==, 0);
+	tt_want(validate_header(&headers, "q", "") == 0);
+	tt_want(validate_header(&headers, "q2", "") == 0);
+	tt_want(validate_header(&headers, "q3", "") == 0);
+	evhttp_clear_headers(&headers);
+
+end:
 	evhttp_clear_headers(&headers);
 }