Skip to content
This repository

Fixed evhttp_parse_query to not fail in particular cases #15

Open
wants to merge 1 commit into from

2 participants

VittGam Nick Mathewson
VittGam

Hi,

I've made those changes to the evhttp_parse_query_impl function to avoid an error on those query strings:

  • test=123&test2 (test2 is now saved as "")
  • test=123&&test2=1 (it no longer crashes on this)
  • test=123&=456&test2=1 (it no longer crashes on this; the no-name entry is discarded)

I also changed this to make the function behave as PHP and others do:

  • test=123&test=456 (test was 123, now is 456)
VittGam VittGam evhttp_parse_query doesn't fail anymore in particular cases
- test=123&test2 (test2 is now saved as "")
- test=123&test=456 (test was 123, now is 456, as PHP and others do)
- test=123&&test2=1 (it no longer crashes on this)
- test=123&=456&test2=1 (it no longer crashes on this; the no-name entry is discarded)
a555ddb
Nick Mathewson
Owner

When I saw that you said "crashes", I freaked out a little, but it turns out that it's just failing. ("Crashing" would mean hitting an assert or a null pointer dereference or something.)

I'm looking at your changes, and I don't understand the reasoning behind all of them. In particular:

  • What standard, if any, specifies query strings like "test=123&test2" ?
  • What standard, if any, allows a query string like "test=123&&test2=1" ?
  • What standard, if any, allows a query string like "test=123&=456&test2=1" ?

As far as I can tell, none of these is standard. Is there some horrible codebase out there that's generating these nonconformant requests that you need to be compatible with or something?

As for "test=123&test=456", is there a standard on that one? I'd hesitate to break existing code that could be relying on the current behavior. It seems to me more reasonable just to include both values in the headers list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

Aug 02, 2012
VittGam VittGam evhttp_parse_query doesn't fail anymore in particular cases
- test=123&test2 (test2 is now saved as "")
- test=123&test=456 (test was 123, now is 456, as PHP and others do)
- test=123&&test2=1 (it no longer crashes on this)
- test=123&=456&test2=1 (it no longer crashes on this; the no-name entry is discarded)
a555ddb
This page is out of date. Refresh to see the latest.

Showing 1 changed file with 10 additions and 2 deletions. Show diff stats Hide diff stats

  1. +10 2 http.c
12 http.c
@@ -2954,18 +2954,26 @@ evhttp_parse_query_impl(const char *str, struct evkeyvalq *headers,
2954 2954
2955 2955 value = argument;
2956 2956 key = strsep(&value, "=");
2957   - if (value == NULL || *key == '\0') {
2958   - goto error;
  2957 + if (*key == '\0') {
  2958 + continue;
  2959 + } else if (value == NULL) {
  2960 + value = "";
2959 2961 }
2960 2962
2961 2963 if ((decoded_value = mm_malloc(strlen(value) + 1)) == NULL) {
2962 2964 event_warn("%s: mm_malloc", __func__);
2963 2965 goto error;
2964 2966 }
  2967 +
2965 2968 evhttp_decode_uri_internal(value, strlen(value),
2966 2969 decoded_value, 1 /*always_decode_plus*/);
  2970 +
2967 2971 event_debug(("Query Param: %s -> %s\n", key, decoded_value));
  2972 +
  2973 + /* make sure that a latter query string parameter overwrites a former one */
  2974 + evhttp_remove_header(headers, key);
2968 2975 evhttp_add_header_internal(headers, key, decoded_value);
  2976 +
2969 2977 mm_free(decoded_value);
2970 2978 }
2971 2979

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.