Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
MIME-Version: 1.0
In-Reply-To: <20161114175911.596523A2832@svn01-us-west.apache.org>
References: <20161114175911.596523A2832@svn01-us-west.apache.org>
From: Eric Covener <covener@gmail.com>
Date: Thu, 22 Dec 2016 10:29:08 -0500
Message-ID: <CALK=YjO50e8Bc3GAbU=t7v0+ZnRHmeEdo2-g0S8fkCn7_suy_A@mail.gmail.com>
Subject: Re: svn commit: r1769669 [2/2] - in /httpd/httpd/branches/2.4.x-merge-http-strict:
 ./ docs/manual/ docs/manual/mod/ include/ server/
To: Apache HTTP Server Development List <dev@httpd.apache.org>
Cc: cvs@httpd.apache.org
Content-Type: text/plain; charset=UTF-8
archived-at: Thu, 22 Dec 2016 15:29:14 -0000

I think the log severity changes below could use some eyes, especially
in context of 2.2.  Are these lowered because they're redundant?  I
haven't yet looked.

I am tempted to leave the old severities for 2.2 and wait and see if
it's confusing in 2.4 (should not have to enable DEBUG to see the
cause of a 400 error)


> @@ -937,7 +1010,7 @@ AP_DECLARE(void) ap_get_mime_headers_cor
>
>              if (last_field == NULL) {
>                  r->status = HTTP_BAD_REQUEST;
> -                ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03442)
> +                ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03442)
>                                "Line folding encountered before first"
>                                " header line");
>                  return;
> @@ -945,7 +1018,7 @@ AP_DECLARE(void) ap_get_mime_headers_cor
>
>              if (field[1] == '\0') {
>                  r->status = HTTP_BAD_REQUEST;
> -                ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03443)
> +                ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03443)
>                                "Empty folded line encountered");
>                  return;
>              }
> @@ -991,9 +1064,7 @@ AP_DECLARE(void) ap_get_mime_headers_cor
>              }
>              memcpy(last_field + last_len, field, len +1); /* +1 for nul */
>              /* Replace obs-fold w/ SP per RFC 7230 3.2.4 */
> -            if (strict || strictspaces) {
> -                last_field[last_len] = ' ';
> -            }
> +            last_field[last_len] = ' ';
>              last_len += len;
>
>              /* We've appended this obs-fold line to last_len, proceed to
> @@ -1024,22 +1095,9 @@ AP_DECLARE(void) ap_get_mime_headers_cor
>              {
>                  /* Not Strict ('Unsafe' mode), using the legacy parser */
>
> -                if (strictspaces && strpbrk(last_field, "\n\v\f\r")) {
> -                    r->status = HTTP_BAD_REQUEST;
> -                    ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03451)
> -                                  "Request header presented bad whitespace "
> -                                  "(disallowed by StrictWhitespace)");
> -                    return;
> -                }
> -                else {
> -                    char *ll = last_field;
> -                    while ((ll = strpbrk(ll, "\n\v\f\r")))
> -                        *(ll++) = ' ';
> -                }
> -
>                  if (!(value = strchr(last_field, ':'))) { /* Find ':' or */
>                      r->status = HTTP_BAD_REQUEST;   /* abort bad request */
> -                    ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00564)
> +                    ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00564)
>                                    "Request header field is missing ':' "
>                                    "separator: %.*s", (int)LOG_NAME_MAX_LEN,
>                                    last_field);
> @@ -1051,11 +1109,11 @@ AP_DECLARE(void) ap_get_mime_headers_cor
>
>                  *value++ = '\0'; /* NUL-terminate at colon */
>
> -                if (strictspaces && strpbrk(last_field, " \t")) {
> +                if (strpbrk(last_field, "\t\n\v\f\r ")) {
>                      r->status = HTTP_BAD_REQUEST;
> -                    ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03452)
> -                                  "Request header field name with whitespace "
> -                                  "(disallowed by StrictWhitespace)");
> +                    ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03452)
> +                                  "Request header field name presented"
> +                                  " invalid whitespace");
>                      return;
>                  }
>
> @@ -1063,15 +1121,17 @@ AP_DECLARE(void) ap_get_mime_headers_cor
>                       ++value;            /* Skip to start of value   */
>                  }
>
> -                /* Strip LWS after field-name: */
> -                while (tmp_field > last_field
> -                           && (*tmp_field == ' ' || *tmp_field == '\t')) {
> -                    *(tmp_field--) = '\0';
> +                if (strpbrk(value, "\n\v\f\r")) {
> +                    r->status = HTTP_BAD_REQUEST;
> +                    ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03451)
> +                                  "Request header field value presented"
> +                                  " bad whitespace");
> +                    return;
>                  }
>
>                  if (tmp_field == last_field) {
>                      r->status = HTTP_BAD_REQUEST;
> -                    ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03453)
> +                    ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03453)
>                                    "Request header field name was empty");
>                      return;
>                  }
> @@ -1082,7 +1142,7 @@ AP_DECLARE(void) ap_get_mime_headers_cor
>                  value = (char *)ap_scan_http_token(last_field);
>                  if ((value == last_field) || *value != ':') {
>                      r->status = HTTP_BAD_REQUEST;
> -                    ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02426)
> +                    ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02426)
>                                    "Request header field name is malformed: "
>                                    "%.*s", (int)LOG_NAME_MAX_LEN, last_field);
>                      return;
> @@ -1104,7 +1164,7 @@ AP_DECLARE(void) ap_get_mime_headers_cor
>                   */
>                  if (*tmp_field != '\0') {
>                      r->status = HTTP_BAD_REQUEST;
> -                    ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02427)
> +                    ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02427)
>                                    "Request header value is malformed: "
>                                    "%.*s", (int)LOG_NAME_MAX_LEN, value);
>                      return;
> @@ -1225,7 +1285,7 @@ request_rec *ap_read_request(conn_rec *c
>                                r->server->limit_req_line);
>              }
>              else if (r->method == NULL) {
> -                ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00566)
> +                ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00566)
>                                "request failed: malformed request line");
>              }
>              access_status = r->status;
> @@ -1265,7 +1325,7 @@ request_rec *ap_read_request(conn_rec *c
>
>          ap_get_mime_headers_core(r, tmp_bb);
>          if (r->status != HTTP_OK) {
> -            ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00567)
> +            ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00567)
>                            "request failed: error reading the headers");
>              ap_send_error_response(r, 0);
>              ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, r);
> @@ -1284,7 +1344,7 @@ request_rec *ap_read_request(conn_rec *c
>               */
>              if (!(strcasecmp(tenc, "chunked") == 0 /* fast path */
>                      || ap_find_last_token(r->pool, tenc, "chunked"))) {
> -                ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02539)
> +                ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02539)
>                                "client sent unknown Transfer-Encoding "
>                                "(%s): %s", tenc, r->uri);
>                  r->status = HTTP_BAD_REQUEST;
> @@ -1305,25 +1365,6 @@ request_rec *ap_read_request(conn_rec *c
>              apr_table_unset(r->headers_in, "Content-Length");
>          }
>      }
> -    else {
> -        if (r->header_only) {
> -            /*
> -             * Client asked for headers only with HTTP/0.9, which doesn't send
> -             * headers! Have to dink things just to make sure the error message
> -             * comes through...
> -             */
> -            ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00568)
> -                          "client sent invalid HTTP/0.9 request: HEAD %s",
> -                          r->uri);
> -            r->header_only = 0;
> -            r->status = HTTP_BAD_REQUEST;
> -            ap_send_error_response(r, 0);
> -            ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, r);
> -            ap_run_log_transaction(r);
> -            apr_brigade_destroy(tmp_bb);
> -            goto traceout;
> -        }
> -    }
>
>      apr_brigade_destroy(tmp_bb);
>
> @@ -1355,7 +1396,7 @@ request_rec *ap_read_request(conn_rec *c
>           * a Host: header, and the server MUST respond with 400 if it doesn't.
>           */
>          access_status = HTTP_BAD_REQUEST;
> -        ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00569)
> +        ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00569)
>                        "client sent HTTP/1.1 request without hostname "
>                        "(see RFC2616 section 14.23): %s", r->uri);
>      }