You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "Roy T. Fielding" <fi...@gbiv.com> on 2013/09/17 21:10:22 UTC
Re: svn commit: r1524161 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number modules/http/http_filters.c server/protocol.c
Umm, 2616 hasn't been the spec-of-record for HTTP/1.1 for almost
six years. The current spec says
If a Transfer-Encoding header field
is present in a request and the chunked transfer coding is not
the final encoding, the message body length cannot be determined
reliably; the server MUST respond with the 400 (Bad Request)
status code and then close the connection.
http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23#page-31
Note that this impacts the following bits in protocol.c
(because the filter doesn't know whether it is reading a
request or a response, so we have to check here):
On Sep 17, 2013, at 11:37 AM, jim@apache.org wrote:
> Modified: httpd/httpd/trunk/server/protocol.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1524161&r1=1524160&r2=1524161&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/protocol.c (original)
> +++ httpd/httpd/trunk/server/protocol.c Tue Sep 17 18:37:18 2013
> @@ -1091,6 +1091,8 @@ request_rec *ap_read_request(conn_rec *c
> }
>
> if (!r->assbackwards) {
> + const char *tenc, *clen;
> +
> ap_get_mime_headers_core(r, tmp_bb);
> if (r->status != HTTP_OK) {
> ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00567)
> @@ -1102,13 +1104,37 @@ request_rec *ap_read_request(conn_rec *c
> goto traceout;
> }
>
> - if (apr_table_get(r->headers_in, "Transfer-Encoding")
> - && apr_table_get(r->headers_in, "Content-Length")) {
> + if ((tenc = apr_table_get(r->headers_in, "Transfer-Encoding"))) {
> /* 2616 section 4.4, point 3: "if both Transfer-Encoding
> * and Content-Length are received, the latter MUST be
> - * ignored"; so unset it here to prevent any confusion
> - * later. */
> - apr_table_unset(r->headers_in, "Content-Length");
> + * ignored"; unless the former is "identity". So unset
> + * the one concerned here to prevent any confusion later.
> + */
> + if ((clen = apr_table_get(r->headers_in, "Content-Length"))) {
> + if (strcasecmp(tenc, "chunked") == 0) {
> + ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(2537)
> + "client sent both Transfer-Encoding (chunked)"
> + " and Content-Length; using TE");
> + apr_table_unset(r->headers_in, "Content-Length");
> + }
> + else {
> + ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(2538)
> + "client sent identity or unknown Transfer-Encoding (%s);"
> + " using Content-Length", tenc);
> + apr_table_unset(r->headers_in, "Transfer-Encoding");
> + }
> + }
> + else if (strcasecmp(tenc, "chunked") != 0) {
> + ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(2539)
> + "client sent unknown Transfer-Encoding;"
> + " not implemented: %s", tenc);
> + r->status = HTTP_NOT_IMPLEMENTED;
> + 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;
> + }
> }
> }
> else {
Almost done,
....Roy
Re: svn commit: r1524161 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number modules/http/http_filters.c server/protocol.c
Posted by Yann Ylavic <yl...@gmail.com>.
S
ame patch plus a (missing) log when the request is blocked in
ap_read_request and minus a spurious semicolon unintentionally added in
ap_http_filter's log.
On Wed, Sep 18, 2013 at 1:14 PM, Jim Jagielski <ji...@jagunet.com> wrote:
> Roy?
>
> On Sep 17, 2013, at 7:07 PM, Yann Ylavic <yl...@gmail.com> wrote:
>
> > The following patch seems to be "draft-ietf-httpbis-p1-messaging-23,
> > section 3.3.3.3" compliant (unlike current code) :
> >
> > </PATCH>
> > Index: server/protocol.c
> > ===================================================================
> > --- server/protocol.c (revision 1524231)
> > +++ server/protocol.c (working copy)
> > @@ -1091,6 +1091,8 @@ request_rec *ap_read_request(conn_rec *conn)
> > }
> >
> > if (!r->assbackwards) {
> > + const char *tenc;
> > +
> > ap_get_mime_headers_core(r, tmp_bb);
> > if (r->status != HTTP_OK) {
> > ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00567)
> > @@ -1102,14 +1104,30 @@ request_rec *ap_read_request(conn_rec *conn)
> > goto traceout;
> > }
> >
> > - if (apr_table_get(r->headers_in, "Transfer-Encoding")
> > - && apr_table_get(r->headers_in, "Content-Length")) {
> > - /*
> > http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23#page-31
> > - * "If a message is received with both a Transfer-Encoding
> and a
> > - * Content-Length header field, the Transfer-Encoding
> overrides the
> > - * Content-Length. ... A sender MUST remove the received
> Content-
> > - * Length field"
> > + tenc = apr_table_get(r->headers_in, "Transfer-Encoding");
> > + if (tenc) {
> > + /*
> http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23
> > + * Section 3.3.3.3: "If a Transfer-Encoding header field is
> > + * present in a request and the chunked transfer coding is
> not
> > + * the final encoding ...; the server MUST respond with the
> 400
> > + * (Bad Request) status code and then close the connection".
> > */
> > + if (strcasecmp(tenc, "chunked") != 0
> > + && !ap_find_last_token(r->pool, tenc, "chunked")) {
> > + 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;
> > + }
> > +
> > + /*
> http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23
> > + * Section 3.3.3.3: "If a message is received with both a
> > + * Transfer-Encoding and a Content-Length header field, the
> > + * Transfer-Encoding overrides the Content-Length. ... A
> sender
> > + * MUST remove the received Content-Length field".
> > + */
> > apr_table_unset(r->headers_in, "Content-Length");
> > }
> > }
> > Index: modules/http/http_filters.c
> > ===================================================================
> > --- modules/http/http_filters.c (revision 1524231)
> > +++ modules/http/http_filters.c (working copy)
> > @@ -224,23 +224,30 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bu
> > lenp = apr_table_get(f->r->headers_in, "Content-Length");
> >
> > if (tenc) {
> > - if (!strcasecmp(tenc, "chunked")) {
> > + if (strcasecmp(tenc, "chunked") == 0
> > + || ap_find_last_token(f->r->pool, tenc, "chunked"))
> {
> > ctx->state = BODY_CHUNK;
> > }
> > - /* test lenp, because it gives another case we can handle */
> > - else if (!lenp) {
> > - /* Something that isn't in HTTP, unless some future
> > + else if (f->r->proxyreq != PROXYREQ_RESPONSE) {
> > + /* Something that isn't a HTTP request, unless some
> future
> > * edition defines new transfer encodings, is
> unsupported.
> > */
> > ap_log_rerror(
> > - APLOG_MARK, APLOG_INFO, 0, f->r,
> > APLOGNO(01585) "Unknown Transfer-Encoding: %s", tenc);
> > + APLOG_MARK, APLOG_INFO, 0, f->r,
> > APLOGNO(01585) "Unknown Transfer-Encoding: %s; ", tenc);
> > return APR_ENOTIMPL;
> > }
> > else {
> > + /*
> > http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23
> > + * Section 3.3.3.3: "If a Transfer-Encoding header
> field is
> > + * present in a response and the chunked transfer
> coding is not
> > + * the final encoding, the message body length is
> determined by
> > + * reading the connection until it is closed by the
> server."
> > + */
> > ap_log_rerror(
> > - APLOG_MARK, APLOG_WARNING, 0, f->r,
> > APLOGNO(01586) "Unknown Transfer-Encoding: %s; using Content-Length",
> > tenc);
> > + APLOG_MARK, APLOG_WARNING, 0, f->r,
> > APLOGNO(01586) "Unknown Transfer-Encoding: %s; using closed
> > connection", tenc);
> > tenc = NULL;
> > }
> > + lenp = NULL;
> > }
> > if (lenp && !tenc) {
> > char *endstr;
> > </PATCH>
> > <draft-ietf-httpbis-p1-messaging-23-section-3.3.3.3.patch>
>
>
Re: svn commit: r1524161 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number modules/http/http_filters.c server/protocol.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
Roy?
On Sep 17, 2013, at 7:07 PM, Yann Ylavic <yl...@gmail.com> wrote:
> The following patch seems to be "draft-ietf-httpbis-p1-messaging-23,
> section 3.3.3.3" compliant (unlike current code) :
>
> </PATCH>
> Index: server/protocol.c
> ===================================================================
> --- server/protocol.c (revision 1524231)
> +++ server/protocol.c (working copy)
> @@ -1091,6 +1091,8 @@ request_rec *ap_read_request(conn_rec *conn)
> }
>
> if (!r->assbackwards) {
> + const char *tenc;
> +
> ap_get_mime_headers_core(r, tmp_bb);
> if (r->status != HTTP_OK) {
> ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00567)
> @@ -1102,14 +1104,30 @@ request_rec *ap_read_request(conn_rec *conn)
> goto traceout;
> }
>
> - if (apr_table_get(r->headers_in, "Transfer-Encoding")
> - && apr_table_get(r->headers_in, "Content-Length")) {
> - /*
> http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23#page-31
> - * "If a message is received with both a Transfer-Encoding and a
> - * Content-Length header field, the Transfer-Encoding overrides the
> - * Content-Length. ... A sender MUST remove the received Content-
> - * Length field"
> + tenc = apr_table_get(r->headers_in, "Transfer-Encoding");
> + if (tenc) {
> + /* http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23
> + * Section 3.3.3.3: "If a Transfer-Encoding header field is
> + * present in a request and the chunked transfer coding is not
> + * the final encoding ...; the server MUST respond with the 400
> + * (Bad Request) status code and then close the connection".
> */
> + if (strcasecmp(tenc, "chunked") != 0
> + && !ap_find_last_token(r->pool, tenc, "chunked")) {
> + 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;
> + }
> +
> + /* http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23
> + * Section 3.3.3.3: "If a message is received with both a
> + * Transfer-Encoding and a Content-Length header field, the
> + * Transfer-Encoding overrides the Content-Length. ... A sender
> + * MUST remove the received Content-Length field".
> + */
> apr_table_unset(r->headers_in, "Content-Length");
> }
> }
> Index: modules/http/http_filters.c
> ===================================================================
> --- modules/http/http_filters.c (revision 1524231)
> +++ modules/http/http_filters.c (working copy)
> @@ -224,23 +224,30 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bu
> lenp = apr_table_get(f->r->headers_in, "Content-Length");
>
> if (tenc) {
> - if (!strcasecmp(tenc, "chunked")) {
> + if (strcasecmp(tenc, "chunked") == 0
> + || ap_find_last_token(f->r->pool, tenc, "chunked")) {
> ctx->state = BODY_CHUNK;
> }
> - /* test lenp, because it gives another case we can handle */
> - else if (!lenp) {
> - /* Something that isn't in HTTP, unless some future
> + else if (f->r->proxyreq != PROXYREQ_RESPONSE) {
> + /* Something that isn't a HTTP request, unless some future
> * edition defines new transfer encodings, is unsupported.
> */
> ap_log_rerror(
> - APLOG_MARK, APLOG_INFO, 0, f->r,
> APLOGNO(01585) "Unknown Transfer-Encoding: %s", tenc);
> + APLOG_MARK, APLOG_INFO, 0, f->r,
> APLOGNO(01585) "Unknown Transfer-Encoding: %s; ", tenc);
> return APR_ENOTIMPL;
> }
> else {
> + /*
> http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23
> + * Section 3.3.3.3: "If a Transfer-Encoding header field is
> + * present in a response and the chunked transfer coding is not
> + * the final encoding, the message body length is determined by
> + * reading the connection until it is closed by the server."
> + */
> ap_log_rerror(
> - APLOG_MARK, APLOG_WARNING, 0, f->r,
> APLOGNO(01586) "Unknown Transfer-Encoding: %s; using Content-Length",
> tenc);
> + APLOG_MARK, APLOG_WARNING, 0, f->r,
> APLOGNO(01586) "Unknown Transfer-Encoding: %s; using closed
> connection", tenc);
> tenc = NULL;
> }
> + lenp = NULL;
> }
> if (lenp && !tenc) {
> char *endstr;
> </PATCH>
> <draft-ietf-httpbis-p1-messaging-23-section-3.3.3.3.patch>
Re: svn commit: r1524161 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number modules/http/http_filters.c server/protocol.c
Posted by Yann Ylavic <yl...@gmail.com>.
Oups, I pulled the discussion off the list, sorry.
On Wed, Sep 18, 2013 at 2:57 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Wed, Sep 18, 2013 at 2:27 PM, Yann Ylavic <yl...@gmail.com> wrote:
>
>> On Wed, Sep 18, 2013 at 2:06 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>>
>>>
>>> On Sep 17, 2013, at 7:07 PM, Yann Ylavic <yl...@gmail.com> wrote:
>>> > }
>>> > - /* test lenp, because it gives another case we can handle
>>> */
>>> > - else if (!lenp) {
>>> > - /* Something that isn't in HTTP, unless some future
>>> > + else if (f->r->proxyreq != PROXYREQ_RESPONSE) {
>>> > + /* Something that isn't a HTTP request, unless some
>>> future
>>> > * edition defines new transfer encodings, is
>>> unsupported.
>>> > */
>>>
>>>
>>> ??
>>>
>>
>> When mod_proxy_http handles a response, r->proxyreq == PROXYREQ_RESPONSE,
>> in any other case (or I'm missing something), ap_http_filter is called for
>> a request.
>>
>> The draft states that :
>>
>> If a Transfer-Encoding header field is present in a response and
>> the chunked transfer coding is not the final encoding, the
>> message body length is determined by reading the connection until
>> it is closed by the server. If a Transfer-Encoding header field
>> is present in a request and the chunked transfer coding is not
>> the final encoding, the message body length cannot be determined
>> reliably; the server MUST respond with the 400 (Bad Request)
>> status code and then close the connection.
>>
>>
>> So this code checks whether the filter is handling a request or a
>> response, in the former case it denies (APR_ENOTIMPL) the transfer-coding,
>> whereas in the latter case it falls through until connection closed
>> (filter's state BODY_NONE).
>>
>
> I should add that ap_read_request in never called for anything else than a
> request, so the check request/response is not necessary there...
>
Re: svn commit: r1524161 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number modules/http/http_filters.c server/protocol.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Sep 18, 2013 at 4:39 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Wed, Sep 18, 2013 at 3:36 PM, Yann Ylavic <yl...@gmail.com> wrote:
>
>> <...> but I'm not sure if the Content-Length of the fake request_rec used
>> here by mod_proxy is forwarded to the client, I'll check that!).
>>
>
> It is not, mod_proxy_http does the right thing :
>
> /* can't have both Content-Length and Transfer-Encoding */
> if (apr_table_get(r->headers_out, "Transfer-Encoding")
> && apr_table_get(r->headers_out, "Content-Length")) {
> /*
>
> * 2616 section 4.4, point 3: "if both Transfer-Encoding
> * and Content-Length are received, the latter MUST be
> * ignored";
> *
> * To help mitigate HTTP Splitting, unset Content-Length
> * and shut down the backend server connection
> * XXX: We aught to treat such a response as uncachable
> */
> apr_table_unset(r->headers_out, "Content-Length");
> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01107)
> "server %s:%d returned Transfer-Encoding"
> " and Content-Length",
> backend->hostname, backend->port);
> backend->close = 1;
> }
>
> Hence the Content-Length is removed from the headers sent to the client
> (r->headers_out) but left in the headers received from the backend
> (backend->r->headers_in), so the ap_http_filter will do its job (check the
> transfer-encoding and reject whatever is not acceptable, incliding both
> TE+CL if "ougth to"), while the client will never receive both
> Content-Length and Transfer-Encoding.
>
>
But it can't hurt to strip the Content-Length from the headers checked by
the filter when applyable, should it be used elsewhere...
Re: svn commit: r1524161 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number modules/http/http_filters.c server/protocol.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Sep 18, 2013 at 3:36 PM, Yann Ylavic <yl...@gmail.com> wrote:
> <...> but I'm not sure if the Content-Length of the fake request_rec used
> here by mod_proxy is forwarded to the client, I'll check that!).
>
It is not, mod_proxy_http does the right thing :
/* can't have both Content-Length and Transfer-Encoding */
if (apr_table_get(r->headers_out, "Transfer-Encoding")
&& apr_table_get(r->headers_out, "Content-Length")) {
/*
* 2616 section 4.4, point 3: "if both Transfer-Encoding
* and Content-Length are received, the latter MUST be
* ignored";
*
* To help mitigate HTTP Splitting, unset Content-Length
* and shut down the backend server connection
* XXX: We aught to treat such a response as uncachable
*/
apr_table_unset(r->headers_out, "Content-Length");
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01107)
"server %s:%d returned Transfer-Encoding"
" and Content-Length",
backend->hostname, backend->port);
backend->close = 1;
}
Hence the Content-Length is removed from the headers sent to the client
(r->headers_out) but left in the headers received from the backend
(backend->r->headers_in), so the ap_http_filter will do its job (check the
transfer-encoding and reject whatever is not acceptable, incliding both
TE+CL if "ougth to"), while the client will never receive both
Content-Length and Transfer-Encoding.
Re: svn commit: r1524161 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number modules/http/http_filters.c server/protocol.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Sep 18, 2013 at 3:55 PM, Yann Ylavic <yl...@gmail.com> wrote:
> However, if the case "ought to be handled as an error", it should rather
> be (in ap_http_filter) :
>
>
> if ((f->r->proxyreq != PROXYREQ_RESPONSE) || lenp) {
>
Well, rather :
if (!lenp
&& (strcasecmp(tenc, "chunked") == 0 // fast path
|| ap_find_last_token(f->r->pool, tenc,
"chunked"))) {
ctx->state = BODY_CHUNK;
}
else if (lenp || f->r->proxyreq != PROXYREQ_RESPONSE) {
or something equivalent (and nicer).
Re: svn commit: r1524161 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number modules/http/http_filters.c server/protocol.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Sep 18, 2013 at 3:36 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Wed, Sep 18, 2013 at 3:02 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>
>> But why also drop the (!lenp) check?
>>
>> Should it be else
>>
>>
>>
>>
>>
>> if ((f->r->proxyreq != PROXYREQ_RESPONSE) || (!lenp)) {
>>
>
> I don't see in the draft where the Content-Length has to be taken into
> account whenever a Transfer-Encoding is specified.
> If the latter is "valid", the former MUST be ignored/stripped :
>
> If a message is received with both a Transfer-Encoding and a
> Content-Length header field, the Transfer-Encoding overrides the
> Content-Length. Such a message might indicate an attempt to
> perform request or response smuggling (bypass of security-related
> checks on message routing or content) and thus ought to be
> handled as an error. A sender MUST remove the received Content-
> Length field prior to forwarding such a message downstream.
>
>
> So I don't see the point for the Content-Length, if it is a request
> whether or not a Content-Length is given a 400 is replied, if it is a
> response the Content-Length must be ignored (and stripped, what the patch
> does not, but I'm not sure if the Content-Length of the fake request_rec
> used here by mod_proxy is forwarded to the client, I'll check that!).
>
However, if the case "ought to be handled as an error", it should rather
be (in ap_http_filter) :
if ((f->r->proxyreq != PROXYREQ_RESPONSE) || lenp) {
and (in ap_read_request) :
if ((strcasecmp(tenc, "chunked") != 0 // fast path
&& !ap_find_last_token(r->pool, tenc, "chunked"))
|| apr_table_get(r->headers_in, "Content-Length")) {
Re: svn commit: r1524161 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number modules/http/http_filters.c server/protocol.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Sep 17, 2013, at 7:07 PM, Yann Ylavic <yl...@gmail.com> wrote:
> }
> - /* test lenp, because it gives another case we can handle */
> - else if (!lenp) {
> - /* Something that isn't in HTTP, unless some future
> + else if (f->r->proxyreq != PROXYREQ_RESPONSE) {
> + /* Something that isn't a HTTP request, unless some future
> * edition defines new transfer encodings, is unsupported.
> */
??
Re: svn commit: r1524161 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number modules/http/http_filters.c server/protocol.c
Posted by Yann Ylavic <yl...@gmail.com>.
Sorry for the disorder gmail made from my previous message, regarding the
changes, I re-post :
On Thu, Sep 19, 2013 at 6:08 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Thu, Sep 19, 2013 at 5:32 PM, Plüm, Rüdiger, Vodafone Group <
> ruediger.pluem@vodafone.com> wrote:
>
>> What did you change from the original patch?
>>
>
> I changed :
> - conn->keepalive = AP_CONN_CLOSE
> ; according to your comment,
> -
> APR_ENOTIMPL => APR_EGENERAL
>
> in ap_http_filter
> ;
>
> so that ap_map_http_request_error
>
> returns 400 and not 501,
> - APLOG_WARNING => APLOG_INFO
> ; when "using read-until-close"
> ,
>
> - (strcasecmp(tenc, "chunked") != 0 && !ap_find_last_token(f->r->pool,
> tenc, "chunked"))
>
> => !(strcasecmp(tenc, "chunked") == 0 ||
> ap_find_last_token(f->r->pool, tenc, "chunked"));
> this one shouldn"t harm and is more what the comment (draft) says
>
> .
>
> - if (...) { fall through } else if (f->r->proxyreq != PROXYREQ_RESPONSE)
> { return EGENERAL } else { fall through }
> => if (...) { fall through } else if (f->r->proxyreq ==
> PROXYREQ_RESPONSE) { fall through } else { return EGENERAL };
> shouldn't harm either, just to end with the failure condition.
>
Re: svn commit: r1524161 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number modules/http/http_filters.c server/protocol.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Sep 19, 2013 at 5:32 PM, Plüm, Rüdiger, Vodafone Group <
ruediger.pluem@vodafone.com> wrote:
> What did you change from the original patch?
>
I changed :
- conn->keepalive = AP_CONN_CLOSE
,
according to your comment
,
-
APR_ENOTIMPL => APR_EGENERAL
in ap_http_filter
so that ap_map_http_request_error
returns 400 and not 501,
- APLOG_WARNING => APLOG_INFO when "using read-until-close".
- (strcasecmp(tenc, "chunked") != 0 && !ap_find_last_token(f->r->pool,
tenc, "chunked")) => !(strcasecmp(tenc, "chunked") == 0 ||
ap_find_last_token(f->r->pool, tenc, "chunked")); this one shouldn"t harm
and is more what the comment (draft) says,
- if (...) { fall through } else if (f->r->proxyreq != PROXYREQ_RESPONSE) {
return EGENERAL } else { fall through } => if (...) { fall through } else
if (f->r->proxyreq == PROXYREQ_RESPONSE) { fall through } else { return
EGENERAL }; shouldn't harm either, just to end with the failure condition.
I don't see why Jim's tests failed, even with the first patch, since the
case does not seem to be tested, so APR_ENOTIMPL => APR_EGENERAL is not hit
(and it's the only change regarding the response status)...
Maybe Jim did have some (other) local changes ?
Regards,
Yann.
AW: svn commit: r1524161 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number modules/http/http_filters.c
server/protocol.c
Posted by Plüm,
Rüdiger,
Vodafone Group <ru...@vodafone.com>.
What did you change from the original patch?
Regards
Rüdiger
> -----Ursprüngliche Nachricht-----
> Von: Jim Jagielski > Gesendet: Donnerstag, 19. September 2013 17:17
> An: dev@httpd.apache.org
> Betreff: Re: svn commit: r1524161 - in /httpd/httpd/trunk: CHANGES
> docs/log-message-tags/next-number modules/http/http_filters.c
> server/protocol.c
>
>
> On Sep 19, 2013, at 9:24 AM, Yann Ylavic <yl...@gmail.com> wrote:
>
> > I can't reproduce, which patch did you use, the latter ?
> >
>
> Yep. This fixes it for me:
>
Re: svn commit: r1524161 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number modules/http/http_filters.c server/protocol.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Sep 19, 2013, at 9:24 AM, Yann Ylavic <yl...@gmail.com> wrote:
> I can't reproduce, which patch did you use, the latter ?
>
Yep. This fixes it for me:
Re: svn commit: r1524161 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number modules/http/http_filters.c server/protocol.c
Posted by Yann Ylavic <yl...@gmail.com>.
I can't reproduce, which patch did you use, the latter ?
t/apache/chunkinput.t ..
1..9
# Running under perl version 5.010001 for linux
# Current time local: Thu Sep 19 15:23:54 2013
# Current time GMT: Thu Sep 19 13:23:54 2013
# Using Test.pm version 1.25_02
# Using Apache/Test.pm version 1.38
testing default
ok 1
# testing : response codes
# expected: HTTP/1.1 200 OK
# received: HTTP/1.1 200 OK
ok 2
# testing : trailer (pid)
# expected: 14984
# received: 14984
ok 3
ok 4
# testing : response codes
# expected: HTTP/1.1 404 Not Found
# received: HTTP/1.1 404 Not Found
ok 5
ok 6
# testing : response codes
# expected: HTTP/1.1 413 Request Entity Too Large
# received: HTTP/1.1 413 Request Entity Too Large
ok 7
ok 8
# testing : response codes
# expected: HTTP/1.1 413 Request Entity Too Large
# received: HTTP/1.1 413 Request Entity Too Large
ok 9
ok
All tests successful.
Files=1, Tests=9, 0 wallclock secs ( 0.02 usr 0.00 sys + 0.41 cusr 0.15
csys = 0.58 CPU)
Result: PASS
On Thu, Sep 19, 2013 at 1:46 PM, Jim Jagielski <ji...@jagunet.com> wrote:
> Unfortunately, it fails tests:
>
> t/apache/chunkinput.t ..
> 1..9
> # Running under perl version 5.012004 for darwin
> # Current time local: Thu Sep 19 07:42:53 2013
> # Current time GMT: Thu Sep 19 11:42:53 2013
> # Using Test.pm version 1.25_02
> # Using Apache/Test.pm version 1.38
> testing default
> ok 1
> # testing : response codes
> # expected: HTTP/1.1 200 OK
> # received: HTTP/1.1 400 Bad Request
> not ok 2
> # testing : trailer (pid)
> # expected: 73948
> # received: <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
> not ok 3
> ok 4
> # Failed test 2 in t/apache/chunkinput.t at line 60
> # Failed test 3 in t/apache/chunkinput.t at line 71
> # testing : response codes
> # expected: HTTP/1.1 404 Not Found
> # received: HTTP/1.1 400 Bad Request
> not ok 5
> # Failed test 5 in t/apache/chunkinput.t at line 60 fail #2
> ok 6
> # testing : response codes
> # expected: HTTP/1.1 413 Request Entity Too Large
> # received: HTTP/1.1 400 Bad Request
> not ok 7
> # Failed test 7 in t/apache/chunkinput.t at line 60 fail #3
> ok 8
> # testing : response codes
> # expected: HTTP/1.1 413 Request Entity Too Large
> # received: HTTP/1.1 400 Bad Request
> not ok 9
> # Failed test 9 in t/apache/chunkinput.t at line 60 fail #4
> Failed 5/9 subtests
>
> Test Summary Report
> -------------------
> t/apache/chunkinput.t (Wstat: 0 Tests: 9 Failed: 5)
> Failed tests: 2-3, 5, 7, 9
> Files=1, Tests=9, 0 wallclock secs ( 0.02 usr 0.01 sys + 0.33 cusr
> 0.18 csys = 0.54 CPU)
> Result: FAIL
> Failed 1/1 test programs. 5/9 subtests failed.
> [warning] server localhost:8529 shutdown
> [ error] error running tests (please examine t/logs/error_log)
>
>
> On Sep 18, 2013, at 7:39 PM, Yann Ylavic <yl...@gmail.com> wrote:
>
> > Thanks,
> >
> > here is the latest patch.
> >
>
>
Re: svn commit: r1524161 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number modules/http/http_filters.c server/protocol.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
Unfortunately, it fails tests:
t/apache/chunkinput.t ..
1..9
# Running under perl version 5.012004 for darwin
# Current time local: Thu Sep 19 07:42:53 2013
# Current time GMT: Thu Sep 19 11:42:53 2013
# Using Test.pm version 1.25_02
# Using Apache/Test.pm version 1.38
testing default
ok 1
# testing : response codes
# expected: HTTP/1.1 200 OK
# received: HTTP/1.1 400 Bad Request
not ok 2
# testing : trailer (pid)
# expected: 73948
# received: <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
not ok 3
ok 4
# Failed test 2 in t/apache/chunkinput.t at line 60
# Failed test 3 in t/apache/chunkinput.t at line 71
# testing : response codes
# expected: HTTP/1.1 404 Not Found
# received: HTTP/1.1 400 Bad Request
not ok 5
# Failed test 5 in t/apache/chunkinput.t at line 60 fail #2
ok 6
# testing : response codes
# expected: HTTP/1.1 413 Request Entity Too Large
# received: HTTP/1.1 400 Bad Request
not ok 7
# Failed test 7 in t/apache/chunkinput.t at line 60 fail #3
ok 8
# testing : response codes
# expected: HTTP/1.1 413 Request Entity Too Large
# received: HTTP/1.1 400 Bad Request
not ok 9
# Failed test 9 in t/apache/chunkinput.t at line 60 fail #4
Failed 5/9 subtests
Test Summary Report
-------------------
t/apache/chunkinput.t (Wstat: 0 Tests: 9 Failed: 5)
Failed tests: 2-3, 5, 7, 9
Files=1, Tests=9, 0 wallclock secs ( 0.02 usr 0.01 sys + 0.33 cusr 0.18 csys = 0.54 CPU)
Result: FAIL
Failed 1/1 test programs. 5/9 subtests failed.
[warning] server localhost:8529 shutdown
[ error] error running tests (please examine t/logs/error_log)
On Sep 18, 2013, at 7:39 PM, Yann Ylavic <yl...@gmail.com> wrote:
> Thanks,
>
> here is the latest patch.
>
Re: svn commit: r1524161 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number modules/http/http_filters.c server/protocol.c
Posted by Yann Ylavic <yl...@gmail.com>.
Thanks,
here is the latest patch.
Note I changed :
-
conn->keepalive = AP_CONN_CLOSE; according to Rüdiger's remark,
- APR_ENOTIMPL => APR_EGENERAL so that ap_map_http_request_error() returns
400 and not 501,
- APLOG_WARNING => APLOG_INFO when "using closed connection".
but didn't changed :
- the APLOGNO(01586) of an existing but modified log, what to do in this
case?
Regards,
Yann.
<PATCH>
Index: server/protocol.c
===================================================================
--- server/protocol.c (revision 1524578)
+++ server/protocol.c (working copy)
@@ -1091,6 +1091,8 @@
}
if (!r->assbackwards) {
+ const char *tenc;
+
ap_get_mime_headers_core(r, tmp_bb);
if (r->status != HTTP_OK) {
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00567)
@@ -1102,14 +1104,34 @@
goto traceout;
}
- if (apr_table_get(r->headers_in, "Transfer-Encoding")
- && apr_table_get(r->headers_in, "Content-Length")) {
- /*
http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23#page-31
- * "If a message is received with both a Transfer-Encoding and
a
- * Content-Length header field, the Transfer-Encoding
overrides the
- * Content-Length. ... A sender MUST remove the received
Content-
- * Length field"
+ tenc = apr_table_get(r->headers_in, "Transfer-Encoding");
+ if (tenc) {
+ /*
http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23
+ * Section 3.3.3.3: "If a Transfer-Encoding header field is
+ * present in a request and the chunked transfer coding is not
+ * the final encoding ...; the server MUST respond with the 400
+ * (Bad Request) status code and then close the connection".
*/
+ 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()
+ "client sent unknown Transfer-Encoding "
+ "(%s): %s", tenc, r->uri);
+ r->status = HTTP_BAD_REQUEST;
+ conn->keepalive = AP_CONN_CLOSE;
+ 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;
+ }
+
+ /*
http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23
+ * Section 3.3.3.3: "If a message is received with both a
+ * Transfer-Encoding and a Content-Length header field, the
+ * Transfer-Encoding overrides the Content-Length. ... A sender
+ * MUST remove the received Content-Length field".
+ */
apr_table_unset(r->headers_in, "Content-Length");
}
}
Index: modules/http/http_filters.c
===================================================================
--- modules/http/http_filters.c (revision 1524578)
+++ modules/http/http_filters.c (working copy)
@@ -224,25 +224,32 @@
lenp = apr_table_get(f->r->headers_in, "Content-Length");
if (tenc) {
- if (!strcasecmp(tenc, "chunked")) {
+ if (strcasecmp(tenc, "chunked") == 0 // fast path
+ || ap_find_last_token(f->r->pool, tenc, "chunked")) {
ctx->state = BODY_CHUNK;
}
- /* test lenp, because it gives another case we can handle */
- else if (!lenp) {
- /* Something that isn't in HTTP, unless some future
- * edition defines new transfer encodings, is unsupported.
+ else if (f->r->proxyreq == PROXYREQ_RESPONSE) {
+ /*
http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23
+ * Section 3.3.3.3: "If a Transfer-Encoding header field is
+ * present in a response and the chunked transfer coding
is not
+ * the final encoding, the message body length is
determined by
+ * reading the connection until it is closed by the
server."
*/
ap_log_rerror(
- APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01585)
"Unknown Transfer-Encoding: %s", tenc);
- return APR_ENOTIMPL;
+ APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01586)
"Unknown Transfer-Encoding: %s; using closed connection", tenc);
+ tenc = NULL;
}
else {
+ /* Something that isn't a HTTP request, unless some future
+ * edition defines new transfer encodings, is unsupported.
+ */
ap_log_rerror(
- APLOG_MARK, APLOG_WARNING, 0, f->r, APLOGNO(01586)
"Unknown Transfer-Encoding: %s; using Content-Length", tenc);
- tenc = NULL;
+ APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01585)
"Unknown Transfer-Encoding: %s", tenc);
+ return APR_EGENERAL;
}
+ lenp = NULL;
}
- if (lenp && !tenc) {
+ else if (lenp) {
char *endstr;
ctx->state = BODY_LENGTH;
</PATCH>
Re: svn commit: r1524161 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number modules/http/http_filters.c server/protocol.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
Agreed.
On Sep 18, 2013, at 3:11 PM, Ruediger Pluem <rp...@apache.org> wrote:
>
>
> Yann Ylavic wrote:
>> The following patch seems to be "draft-ietf-httpbis-p1-messaging-23,
>> section 3.3.3.3" compliant (unlike current code) :
>
> Looks good to me except for one comment.
>
>>
>> </PATCH>
>> Index: server/protocol.c
>> ===================================================================
>> --- server/protocol.c (revision 1524231)
>> +++ server/protocol.c (working copy)
>> @@ -1091,6 +1091,8 @@ request_rec *ap_read_request(conn_rec *conn)
>> }
>>
>> if (!r->assbackwards) {
>> + const char *tenc;
>> +
>> ap_get_mime_headers_core(r, tmp_bb);
>> if (r->status != HTTP_OK) {
>> ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00567)
>> @@ -1102,14 +1104,30 @@ request_rec *ap_read_request(conn_rec *conn)
>> goto traceout;
>> }
>>
>> - if (apr_table_get(r->headers_in, "Transfer-Encoding")
>> - && apr_table_get(r->headers_in, "Content-Length")) {
>> - /*
>> http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23#page-31
>> - * "If a message is received with both a Transfer-Encoding and a
>> - * Content-Length header field, the Transfer-Encoding overrides the
>> - * Content-Length. ... A sender MUST remove the received Content-
>> - * Length field"
>> + tenc = apr_table_get(r->headers_in, "Transfer-Encoding");
>> + if (tenc) {
>> + /* http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23
>> + * Section 3.3.3.3: "If a Transfer-Encoding header field is
>> + * present in a request and the chunked transfer coding is not
>> + * the final encoding ...; the server MUST respond with the 400
>> + * (Bad Request) status code and then close the connection".
>> */
>> + if (strcasecmp(tenc, "chunked") != 0
>> + && !ap_find_last_token(r->pool, tenc, "chunked")) {
>> + r->status = HTTP_BAD_REQUEST;
>
> I think we miss
>
> conn->keepalive = AP_CONN_CLOSE;
>
> here in order to ensure that the connection gets closed after sending the error message.
>
>> + 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;
>> + }
>> +
>> + /* http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23
>> + * Section 3.3.3.3: "If a message is received with both a
>> + * Transfer-Encoding and a Content-Length header field, the
>> + * Transfer-Encoding overrides the Content-Length. ... A sender
>> + * MUST remove the received Content-Length field".
>> + */
>> apr_table_unset(r->headers_in, "Content-Length");
>> }
>> }
>
> Regards
>
> Rüdiger
>
Re: svn commit: r1524161 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number
modules/http/http_filters.c server/protocol.c
Posted by Ruediger Pluem <rp...@apache.org>.
Yann Ylavic wrote:
> The following patch seems to be "draft-ietf-httpbis-p1-messaging-23,
> section 3.3.3.3" compliant (unlike current code) :
Looks good to me except for one comment.
>
> </PATCH>
> Index: server/protocol.c
> ===================================================================
> --- server/protocol.c (revision 1524231)
> +++ server/protocol.c (working copy)
> @@ -1091,6 +1091,8 @@ request_rec *ap_read_request(conn_rec *conn)
> }
>
> if (!r->assbackwards) {
> + const char *tenc;
> +
> ap_get_mime_headers_core(r, tmp_bb);
> if (r->status != HTTP_OK) {
> ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00567)
> @@ -1102,14 +1104,30 @@ request_rec *ap_read_request(conn_rec *conn)
> goto traceout;
> }
>
> - if (apr_table_get(r->headers_in, "Transfer-Encoding")
> - && apr_table_get(r->headers_in, "Content-Length")) {
> - /*
> http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23#page-31
> - * "If a message is received with both a Transfer-Encoding and a
> - * Content-Length header field, the Transfer-Encoding overrides the
> - * Content-Length. ... A sender MUST remove the received Content-
> - * Length field"
> + tenc = apr_table_get(r->headers_in, "Transfer-Encoding");
> + if (tenc) {
> + /* http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23
> + * Section 3.3.3.3: "If a Transfer-Encoding header field is
> + * present in a request and the chunked transfer coding is not
> + * the final encoding ...; the server MUST respond with the 400
> + * (Bad Request) status code and then close the connection".
> */
> + if (strcasecmp(tenc, "chunked") != 0
> + && !ap_find_last_token(r->pool, tenc, "chunked")) {
> + r->status = HTTP_BAD_REQUEST;
I think we miss
conn->keepalive = AP_CONN_CLOSE;
here in order to ensure that the connection gets closed after sending the error message.
> + 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;
> + }
> +
> + /* http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23
> + * Section 3.3.3.3: "If a message is received with both a
> + * Transfer-Encoding and a Content-Length header field, the
> + * Transfer-Encoding overrides the Content-Length. ... A sender
> + * MUST remove the received Content-Length field".
> + */
> apr_table_unset(r->headers_in, "Content-Length");
> }
> }
Regards
Rüdiger
Re: svn commit: r1524161 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number modules/http/http_filters.c server/protocol.c
Posted by Yann Ylavic <yl...@gmail.com>.
The following patch seems to be "draft-ietf-httpbis-p1-messaging-23,
section 3.3.3.3" compliant (unlike current code) :
</PATCH>
Index: server/protocol.c
===================================================================
--- server/protocol.c (revision 1524231)
+++ server/protocol.c (working copy)
@@ -1091,6 +1091,8 @@ request_rec *ap_read_request(conn_rec *conn)
}
if (!r->assbackwards) {
+ const char *tenc;
+
ap_get_mime_headers_core(r, tmp_bb);
if (r->status != HTTP_OK) {
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00567)
@@ -1102,14 +1104,30 @@ request_rec *ap_read_request(conn_rec *conn)
goto traceout;
}
- if (apr_table_get(r->headers_in, "Transfer-Encoding")
- && apr_table_get(r->headers_in, "Content-Length")) {
- /*
http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23#page-31
- * "If a message is received with both a Transfer-Encoding and a
- * Content-Length header field, the Transfer-Encoding overrides the
- * Content-Length. ... A sender MUST remove the received Content-
- * Length field"
+ tenc = apr_table_get(r->headers_in, "Transfer-Encoding");
+ if (tenc) {
+ /* http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23
+ * Section 3.3.3.3: "If a Transfer-Encoding header field is
+ * present in a request and the chunked transfer coding is not
+ * the final encoding ...; the server MUST respond with the 400
+ * (Bad Request) status code and then close the connection".
*/
+ if (strcasecmp(tenc, "chunked") != 0
+ && !ap_find_last_token(r->pool, tenc, "chunked")) {
+ 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;
+ }
+
+ /* http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23
+ * Section 3.3.3.3: "If a message is received with both a
+ * Transfer-Encoding and a Content-Length header field, the
+ * Transfer-Encoding overrides the Content-Length. ... A sender
+ * MUST remove the received Content-Length field".
+ */
apr_table_unset(r->headers_in, "Content-Length");
}
}
Index: modules/http/http_filters.c
===================================================================
--- modules/http/http_filters.c (revision 1524231)
+++ modules/http/http_filters.c (working copy)
@@ -224,23 +224,30 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bu
lenp = apr_table_get(f->r->headers_in, "Content-Length");
if (tenc) {
- if (!strcasecmp(tenc, "chunked")) {
+ if (strcasecmp(tenc, "chunked") == 0
+ || ap_find_last_token(f->r->pool, tenc, "chunked")) {
ctx->state = BODY_CHUNK;
}
- /* test lenp, because it gives another case we can handle */
- else if (!lenp) {
- /* Something that isn't in HTTP, unless some future
+ else if (f->r->proxyreq != PROXYREQ_RESPONSE) {
+ /* Something that isn't a HTTP request, unless some future
* edition defines new transfer encodings, is unsupported.
*/
ap_log_rerror(
- APLOG_MARK, APLOG_INFO, 0, f->r,
APLOGNO(01585) "Unknown Transfer-Encoding: %s", tenc);
+ APLOG_MARK, APLOG_INFO, 0, f->r,
APLOGNO(01585) "Unknown Transfer-Encoding: %s; ", tenc);
return APR_ENOTIMPL;
}
else {
+ /*
http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23
+ * Section 3.3.3.3: "If a Transfer-Encoding header field is
+ * present in a response and the chunked transfer coding is not
+ * the final encoding, the message body length is determined by
+ * reading the connection until it is closed by the server."
+ */
ap_log_rerror(
- APLOG_MARK, APLOG_WARNING, 0, f->r,
APLOGNO(01586) "Unknown Transfer-Encoding: %s; using Content-Length",
tenc);
+ APLOG_MARK, APLOG_WARNING, 0, f->r,
APLOGNO(01586) "Unknown Transfer-Encoding: %s; using closed
connection", tenc);
tenc = NULL;
}
+ lenp = NULL;
}
if (lenp && !tenc) {
char *endstr;
</PATCH>
Re: svn commit: r1524161 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number modules/http/http_filters.c server/protocol.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Sep 17, 2013 at 10:52 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Tue, Sep 17, 2013 at 10:42 PM, Yann Ylavic <yl...@gmail.com>wrote:
>
>> On Tue, Sep 17, 2013 at 9:34 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>>
>>> Ahh... So none of this is really needed then. At least, it should
>>> be reverted.
>>>
>>>
>> Well, as I understand the "new rfc", the old code is broken too.
>>
>> However, the fix should not continue with the content-length (if any)
>> when the transfer-encoding is not (ended-by-)chunked, but return 400
>> instead (or 502 when it applies to a response).
>>
>
> Clearly, return 400/502 whenever an transfer-encoding is given and is not
> (ended-by-)chunked, am I missing something ?
>
Something like :
if ((tenc = apr_table_get(r->headers_in, "Transfer-Encoding"))) {
if (strcasecmp(tenc, "chunked") != 0) {
// ap_read_request => fail with 400
// ap_http_filter => return APR_ENOTIMPL
}
apr_table_unset(r->headers_in, "Content-Length");
}
Re: svn commit: r1524161 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number modules/http/http_filters.c server/protocol.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Sep 17, 2013 at 10:42 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Tue, Sep 17, 2013 at 9:34 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>
>> Ahh... So none of this is really needed then. At least, it should
>> be reverted.
>>
>>
> Well, as I understand the "new rfc", the old code is broken too.
>
> However, the fix should not continue with the content-length (if any) when
> the transfer-encoding is not (ended-by-)chunked, but return 400 instead (or
> 502 when it applies to a response).
>
Clearly, return 400/502 whenever an transfer-encoding is given and is not
(ended-by-)chunked, am I missing something ?
Re: svn commit: r1524161 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number modules/http/http_filters.c server/protocol.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Sep 17, 2013 at 9:34 PM, Jim Jagielski <ji...@jagunet.com> wrote:
> Ahh... So none of this is really needed then. At least, it should
> be reverted.
>
>
Well, as I understand the "new rfc", the old code is broken too.
However, the fix should not continue with the content-length (if any) when
the transfer-encoding is not (ended-by-)chunked, but return 400 instead (or
502 when it applies to a response).
Re: svn commit: r1524161 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number modules/http/http_filters.c server/protocol.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
Ahh... So none of this is really needed then. At least, it should
be reverted.
On Sep 17, 2013, at 3:10 PM, "Roy T. Fielding" <fi...@gbiv.com> wrote:
> Umm, 2616 hasn't been the spec-of-record for HTTP/1.1 for almost
> six years. The current spec says
>
> If a Transfer-Encoding header field
> is present in a request and the chunked transfer coding is not
> the final encoding, the message body length cannot be determined
> reliably; the server MUST respond with the 400 (Bad Request)
> status code and then close the connection.
>
> http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23#page-31
>
> Note that this impacts the following bits in protocol.c
> (because the filter doesn't know whether it is reading a
> request or a response, so we have to check here):
>
> On Sep 17, 2013, at 11:37 AM, jim@apache.org wrote:
>
>> Modified: httpd/httpd/trunk/server/protocol.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1524161&r1=1524160&r2=1524161&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/server/protocol.c (original)
>> +++ httpd/httpd/trunk/server/protocol.c Tue Sep 17 18:37:18 2013
>> @@ -1091,6 +1091,8 @@ request_rec *ap_read_request(conn_rec *c
>> }
>>
>> if (!r->assbackwards) {
>> + const char *tenc, *clen;
>> +
>> ap_get_mime_headers_core(r, tmp_bb);
>> if (r->status != HTTP_OK) {
>> ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00567)
>> @@ -1102,13 +1104,37 @@ request_rec *ap_read_request(conn_rec *c
>> goto traceout;
>> }
>>
>> - if (apr_table_get(r->headers_in, "Transfer-Encoding")
>> - && apr_table_get(r->headers_in, "Content-Length")) {
>> + if ((tenc = apr_table_get(r->headers_in, "Transfer-Encoding"))) {
>> /* 2616 section 4.4, point 3: "if both Transfer-Encoding
>> * and Content-Length are received, the latter MUST be
>> - * ignored"; so unset it here to prevent any confusion
>> - * later. */
>> - apr_table_unset(r->headers_in, "Content-Length");
>> + * ignored"; unless the former is "identity". So unset
>> + * the one concerned here to prevent any confusion later.
>> + */
>> + if ((clen = apr_table_get(r->headers_in, "Content-Length"))) {
>> + if (strcasecmp(tenc, "chunked") == 0) {
>> + ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(2537)
>> + "client sent both Transfer-Encoding (chunked)"
>> + " and Content-Length; using TE");
>> + apr_table_unset(r->headers_in, "Content-Length");
>> + }
>> + else {
>> + ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(2538)
>> + "client sent identity or unknown Transfer-Encoding (%s);"
>> + " using Content-Length", tenc);
>> + apr_table_unset(r->headers_in, "Transfer-Encoding");
>> + }
>> + }
>> + else if (strcasecmp(tenc, "chunked") != 0) {
>> + ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(2539)
>> + "client sent unknown Transfer-Encoding;"
>> + " not implemented: %s", tenc);
>> + r->status = HTTP_NOT_IMPLEMENTED;
>> + 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;
>> + }
>> }
>> }
>> else {
>
> Almost done,
>
> ....Roy