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 af1ff888def98bfad43347d7f66e12f6bbaee539..30b09e9e7bbf957dd7069125a9276ad12977f0ce 100644 (file)
--- 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 cab068a5e9110c220c0bf793df3cea41706fd54c..10822b930b9497b0fb03228476e11cb44dfdf6c5 100644 (file)
--- 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 5b899a23a7981a4b17c85a611e02f3f62731ab23..2554b7dd83a5e0858b7278e967e7856db5ac29d3 100644 (file)
--- 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);
 }