> Am 13.04.2022 um 17:16 schrieb Ruediger Pluem <rpl...@apache.org>:
>
>
>
> On 4/7/22 12:41 PM, ic...@apache.org wrote:
>> Author: icing
>> Date: Thu Apr 7 10:41:46 2022
>> New Revision: 1899648
>>
>> URL: http://svn.apache.org/viewvc?rev=1899648&view=rev
>> Log:
>> *) core/mod_http: use RESPONSE meta buckets and a new HTTP/1.x specific
>> filter to send responses through the output filter chain.
>> Specifically: the HTTP_HEADER output filter and
>> ap_send_interim_response()
>> create a RESPONSE bucket and no longer are concerned with HTTP/1.x
>> serialization.
>> A new HTTP1_RESPONSE_OUT transcode filter writes the proper HTTP/1.x
>> bytes when dealing with a RESPONSE bucket. That filter installs itself
>> on the pre_read_request hook when the connection has protocol 'http/1.1'.
>>
>>
>> Added:
>> httpd/httpd/trunk/changes-entries/core_response_buckets.txt
>> Modified:
>> httpd/httpd/trunk/include/mod_core.h
>> httpd/httpd/trunk/modules/http/http_core.c
>> httpd/httpd/trunk/modules/http/http_filters.c
>> httpd/httpd/trunk/modules/http/http_protocol.c
>> httpd/httpd/trunk/server/protocol.c
>>
>
>> Modified: httpd/httpd/trunk/modules/http/http_filters.c
>> URL:
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=1899648&r1=1899647&r2=1899648&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/http/http_filters.c (original)
>> +++ httpd/httpd/trunk/modules/http/http_filters.c Thu Apr 7 10:41:46 2022
>
>> @@ -1364,62 +1178,124 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>> return rv;
>> }
>> }
>> -
>> - for (e = APR_BRIGADE_FIRST(b);
>> - e != APR_BRIGADE_SENTINEL(b);
>> - e = APR_BUCKET_NEXT(e))
>> - {
>> - if (AP_BUCKET_IS_ERROR(e) && !eb) {
>> - eb = e->data;
>> - continue;
>> - }
>> - if (APR_BUCKET_IS_EOS(e)) {
>> - if (!eos) eos = e;
>> - continue;
>> - }
>> - /*
>> - * If we see an EOC bucket it is a signal that we should get out
>> - * of the way doing nothing.
>> + else {
>> + /* Determine if it is time to insert the response bucket for
>> + * the request. Request handlers just write content or an EOS
>> + * and that needs to take the current state of request_rec to
>> + * send on status and headers as a response bucket.
>> + *
>> + * But we also send interim responses (as response buckets)
>> + * through this filter and that must not trigger generating
>> + * an additional response bucket.
>> + *
>> + * Waiting on a DATA/ERROR/EOS bucket alone is not enough,
>> + * unfortunately, as some handlers trigger response generation
>> + * by just writing a FLUSH (see mod_lua's websocket for example).
>> */
>> - if (AP_BUCKET_IS_EOC(e)) {
>> - ap_remove_output_filter(f);
>> - return ap_pass_brigade(f->next, b);
>> + for (e = APR_BRIGADE_FIRST(b);
>> + e != APR_BRIGADE_SENTINEL(b) && !trigger;
>> + e = APR_BUCKET_NEXT(e))
>> + {
>> + if (AP_BUCKET_IS_RESPONSE(e)) {
>> + /* remember the last one if there are many. */
>> + respb = e;
>> + }
>> + else if (APR_BUCKET_IS_FLUSH(e)) {
>> + /* flush without response bucket triggers */
>> + if (!respb) trigger = e;
>> + }
>> + else if (APR_BUCKET_IS_EOS(e)) {
>> + trigger = e;
>> + }
>> + else if (AP_BUCKET_IS_ERROR(e)) {
>> + /* Need to handle this below via ap_die() */
>> + break;
>> + }
>> + else {
>> + /* First content bucket, always triggering the response.*/
>> + trigger = e;
>> + }
>
> Why don't we remove ourselves any longer if we hit an EOC bucket and pass
> stuff on immediately?
> If we want to handle this HTTP protocol agnostic here we need to ensure that
> the EOC bucket
> triggers the immediate removal of the ap_h1_response_out_filter once the
> ap_h1_response_out_filter
> sees it without putting any headers on the wire.
> I am not sure what the appropriate behaviour would be in the HTTP > 1.1 case.
Valid point. The previous filter implementation removed itself immediately on
encountering an EOC, regardless of where it appeared in the brigade.
The current implementation inserts, if not done previously, a RESPONSE bucket.
Afterwards it checks on EOC, remove itself and passes on the brigade.
The behaviour differs, if an EOC is sent *without* a final RESPONSE having been
created before that. The question is what the intended behaviour of passing an
EOC is. Reading the comment in mod_proxy_http.c:1096, it seems that the
intention really
is to NOT generate a response.
Seems I should change the new implementation to also do that. For H2, not
seeing
a RESPONSE would be a stream error and reset the stream. Which seems fine.
>> }
>> - }
>>
>> - if (!ctx->headers_sent && !check_headers(r)) {
>> - /* We may come back here from ap_die() below,
>> - * so clear anything from this response.
>> - */
>> - apr_table_clear(r->headers_out);
>> - apr_table_clear(r->err_headers_out);
>> - apr_brigade_cleanup(b);
>> + if (respb) {
>> + ap_bucket_response *resp = respb->data;
>> + if (resp->status >= 200 || resp->status == 101) {
>
> Why using the literal 101 instead of HTTP_SWITCHING_PROTOCOLS?
>
>> + /* Someone is passing the final response, remember it
>> + * so we no longer generate one. */
>> + ctx->final_status = resp->status;
>> + ctx->final_header_only =
>> AP_STATUS_IS_HEADER_ONLY(resp->status);
>> + }
>> + }
>>
>
>> @@ -1430,148 +1306,15 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>> }
>>
>> if (eos) {
>> - /* on having seen EOS and added possible trailers, we
>> - * can remove this filter.
>> - */
>> e = create_trailers_bucket(r, b->bucket_alloc);
>> if (e) {
>> APR_BUCKET_INSERT_BEFORE(eos, e);
>> }
>> ap_remove_output_filter(f);
>> }
>> -
>> - if (ctx->headers_sent) {
>> - /* we did already the stuff below, just pass on */
>> - return ap_pass_brigade(f->next, b);
>> - }
>> -
>> - /*
>> - * Now that we are ready to send a response, we need to combine the two
>> - * header field tables into a single table. If we don't do this, our
>> - * later attempts to set or unset a given fieldname might be bypassed.
>> - */
>> - if (!apr_is_empty_table(r->err_headers_out)) {
>> - r->headers_out = apr_table_overlay(r->pool, r->err_headers_out,
>> - r->headers_out);
>> - }
>> -
>> - /*
>> - * Remove the 'Vary' header field if the client can't handle it.
>> - * Since this will have nasty effects on HTTP/1.1 caches, force
>> - * the response into HTTP/1.0 mode.
>> - *
>> - * Note: the force-response-1.0 should come before the call to
>> - * basic_http_header_check()
>> - */
>> - if (apr_table_get(r->subprocess_env, "force-no-vary") != NULL) {
>> - apr_table_unset(r->headers_out, "Vary");
>> - r->proto_num = HTTP_VERSION(1,0);
>> - apr_table_setn(r->subprocess_env, "force-response-1.0", "1");
>> - }
>> - else {
>> - fixup_vary(r);
>> - }
>> -
>> - /*
>> - * Now remove any ETag response header field if earlier processing
>> - * says so (such as a 'FileETag None' directive).
>> - */
>> - if (apr_table_get(r->notes, "no-etag") != NULL) {
>> - apr_table_unset(r->headers_out, "ETag");
>> - }
>> -
>> - /* determine the protocol and whether we should use keepalives. */
>> - basic_http_header_check(r, &protocol);
>> - ap_set_keepalive(r);
>> -
>> - if (AP_STATUS_IS_HEADER_ONLY(r->status)) {
>> - apr_table_unset(r->headers_out, "Transfer-Encoding");
>> - apr_table_unset(r->headers_out, "Content-Length");
>> - r->content_type = r->content_encoding = NULL;
>> - r->content_languages = NULL;
>> - r->clength = r->chunked = 0;
>> - }
>> - else if (r->chunked) {
>> - apr_table_mergen(r->headers_out, "Transfer-Encoding", "chunked");
>> - apr_table_unset(r->headers_out, "Content-Length");
>> - }
>> -
>> - ctype = ap_make_content_type(r, r->content_type);
>> - if (ctype) {
>> - apr_table_setn(r->headers_out, "Content-Type", ctype);
>> - }
>> -
>> - if (r->content_encoding) {
>> - apr_table_setn(r->headers_out, "Content-Encoding",
>> - r->content_encoding);
>> - }
>> -
>> - if (!apr_is_empty_array(r->content_languages)) {
>> - int i;
>> - char *token;
>> - char **languages = (char **)(r->content_languages->elts);
>> - const char *field = apr_table_get(r->headers_out,
>> "Content-Language");
>> -
>> - while (field && (token = ap_get_list_item(r->pool, &field)) !=
>> NULL) {
>> - for (i = 0; i < r->content_languages->nelts; ++i) {
>> - if (!ap_cstr_casecmp(token, languages[i]))
>> - break;
>> - }
>> - if (i == r->content_languages->nelts) {
>> - *((char **) apr_array_push(r->content_languages)) = token;
>> - }
>> - }
>> -
>> - field = apr_array_pstrcat(r->pool, r->content_languages, ',');
>> - apr_table_setn(r->headers_out, "Content-Language", field);
>> - }
>> -
>> - /*
>> - * Control cachability for non-cacheable responses if not already set by
>> - * some other part of the server configuration.
>> - */
>> - if (r->no_cache && !apr_table_get(r->headers_out, "Expires")) {
>> - char *date = apr_palloc(r->pool, APR_RFC822_DATE_LEN);
>> - ap_recent_rfc822_date(date, r->request_time);
>> - apr_table_addn(r->headers_out, "Expires", date);
>> - }
>> -
>> - b2 = apr_brigade_create(r->pool, c->bucket_alloc);
>> - basic_http_header(r, b2, protocol);
>> -
>> - h.pool = r->pool;
>> - h.bb = b2;
>> -
>> - send_all_header_fields(&h, r);
>> -
>> - terminate_header(b2);
>> -
>> - if (header_only) {
>> - e = APR_BRIGADE_LAST(b);
>> - if (e != APR_BRIGADE_SENTINEL(b) && APR_BUCKET_IS_EOS(e)) {
>> - APR_BUCKET_REMOVE(e);
>> - APR_BRIGADE_INSERT_TAIL(b2, e);
>> - ap_remove_output_filter(f);
>> - }
>> - apr_brigade_cleanup(b);
>> - }
>> -
>> - rv = ap_pass_brigade(f->next, b2);
>> - apr_brigade_cleanup(b2);
>> - ctx->headers_sent = 1;
>> -
>> - if (rv != APR_SUCCESS || header_only) {
>> - goto out;
>> - }
>> -
>> - r->sent_bodyct = 1; /* Whatever follows is real body stuff... */
>> -
>> - if (r->chunked) {
>> - /* We can't add this filter until we have already sent the headers.
>> - * If we add it before this point, then the headers will be chunked
>> - * as well, and that is just wrong.
>> - */
>> - ap_add_output_filter("CHUNK", NULL, r, r->connection);
>> + else if (ctx->final_status == 101) {
>
> Why using the literal 101 instead of HTTP_SWITCHING_PROTOCOLS?
The HTTP status numbers are burned into my cortex, but I agree that
using the constant gives better readability. Will change.
>
>> + /* switching protocol, whatever comes next is not HTTP/1.x */
>> + ap_remove_output_filter(f);
>> }
>>
>> rv = ap_pass_brigade(f->next, b);
>> @@ -1989,3 +1732,351 @@ apr_status_t ap_http_outerror_filter(ap_
>>
>> return ap_pass_brigade(f->next, b);
>> }
>> +
>> +
>> +/* fill "bb" with a barebones/initial HTTP/1.x response header */
>> +static void h1_append_response_head(request_rec *r,
>> + ap_bucket_response *resp,
>> + const char *protocol,
>> + apr_bucket_brigade *bb)
>> +{
>> + const char *date = NULL;
>> + const char *server = NULL;
>> + const char *status_line;
>> + struct iovec vec[4];
>> +
>> + if (r->assbackwards) {
>> + /* there are no headers to send */
>> + return;
>> + }
>> +
>> + /* Output the HTTP/1.x Status-Line and the Date and Server fields */
>> + if (resp->reason) {
>> + status_line = apr_psprintf(r->pool, "%d %s", resp->status,
>> resp->reason);
>> + }
>> + else {
>> + status_line = ap_get_status_line_ex(r->pool, resp->status);
>> + }
>> +
>> + vec[0].iov_base = (void *)protocol;
>> + vec[0].iov_len = strlen(protocol);
>> + vec[1].iov_base = (void *)" ";
>> + vec[1].iov_len = sizeof(" ") - 1;
>> + vec[2].iov_base = (void *)(status_line);
>> + vec[2].iov_len = strlen(status_line);
>> + vec[3].iov_base = (void *)CRLF;
>> + vec[3].iov_len = sizeof(CRLF) - 1;
>> +#if APR_CHARSET_EBCDIC
>> + {
>> + char *tmp;
>> + apr_size_t len;
>> + tmp = apr_pstrcatv(r->pool, vec, 4, &len);
>> + ap_xlate_proto_to_ascii(tmp, len);
>> + apr_brigade_write(bb, NULL, NULL, tmp, len);
>> + }
>> +#else
>> + apr_brigade_writev(bb, NULL, NULL, vec, 4);
>> +#endif
>> +
>> + date = apr_table_get(resp->headers, "Date");
>> + server = apr_table_get(resp->headers, "Server");
>> + if (date) {
>> + /* We always write that as first, just because we
>> + * always did and some quirky clients might rely on that.
>> + */
>> + ap_h1_append_header(bb, r->pool, "Date", date);
>> + apr_table_unset(resp->headers, "Date");
>> + }
>> + if (server) {
>> + /* We always write that second, just because we
>> + * always did and some quirky clients might rely on that.
>> + */
>> + ap_h1_append_header(bb, r->pool, "Server", server);
>> + apr_table_unset(resp->headers, "Server");
>> + }
>> +
>> + if (APLOGrtrace3(r)) {
>> + ap_log_rerror(APLOG_MARK, APLOG_TRACE3, 0, r,
>> + "Response sent with status %d%s",
>> + r->status,
>> + APLOGrtrace4(r) ? ", headers:" : "");
>> +
>> + /*
>> + * Date and Server are less interesting, use TRACE5 for them while
>> + * using TRACE4 for the other headers.
>> + */
>> + if (date)
>> + ap_log_rerror(APLOG_MARK, APLOG_TRACE5, 0, r, " Date: %s",
>> + date);
>> + if (server)
>> + ap_log_rerror(APLOG_MARK, APLOG_TRACE5, 0, r, " Server: %s",
>> + server);
>> + }
>> +}
>> +
>> +typedef struct h1_response_ctx {
>> + int final_response_sent; /* strict: a response status >= 200 was
>> sent */
>> + int discard_body; /* the response is header only, discard
>> body */
>> + apr_bucket_brigade *tmpbb;
>> +} h1_response_ctx;
>> +
>> +AP_CORE_DECLARE_NONSTD(apr_status_t) ap_h1_response_out_filter(ap_filter_t
>> *f,
>> +
>> apr_bucket_brigade *b)
>> +{
>> + request_rec *r = f->r;
>> + conn_rec *c = r->connection;
>> + apr_bucket *e, *next = NULL;
>> + h1_response_ctx *ctx = f->ctx;
>> + apr_status_t rv = APR_SUCCESS;
>> + core_server_config *conf =
>> ap_get_core_module_config(r->server->module_config);
>> + int strict = (conf->http_conformance != AP_HTTP_CONFORMANCE_UNSAFE);
>> +
>> + AP_DEBUG_ASSERT(!r->main);
>> +
>> + if (!ctx) {
>> + ctx = f->ctx = apr_pcalloc(r->pool, sizeof(*ctx));
>> + }
>> +
>> + for (e = APR_BRIGADE_FIRST(b);
>> + e != APR_BRIGADE_SENTINEL(b);
>> + e = next)
>> + {
>> + next = APR_BUCKET_NEXT(e);
>> +
>> + if (APR_BUCKET_IS_METADATA(e)) {
>> +
>> + if (APR_BUCKET_IS_EOS(e)) {
>> + if (!ctx->final_response_sent) {
>> + /* should not happen. do we generate a 500 here? */
>> + }
>> + ap_remove_output_filter(f);
>> + goto pass;
>> + }
>> + else if (AP_BUCKET_IS_RESPONSE(e)) {
>> + ap_bucket_response *resp = e->data;
>> +
>> + ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
>> + "ap_http1_response_out_filter seeing response
>> bucket status=%d",
>> + resp->status);
>> + if (strict && resp->status < 100) {
>> + /* error, not a valid http status */
>> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
>> APLOGNO(10386)
>> + "ap_http1_response_out_filter seeing
>> headers "
>> + "status=%d in strict mode",
>> + resp->status);
>> + rv = AP_FILTER_ERROR;
>> + goto cleanup;
>> + }
>> + else if (ctx->final_response_sent) {
>> + /* already sent the final response for the request.
>> + */
>> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
>> APLOGNO(10387)
>> + "ap_http1_response_out_filter seeing
>> headers "
>> + "status=%d after final response already
>> sent",
>> + resp->status);
>> + rv = AP_FILTER_ERROR;
>> + goto cleanup;
>> + }
>> + else {
>> + /* a response status to transcode, might be final or
>> interim
>> + */
>> + const char *proto = AP_SERVER_PROTOCOL;
>> +
>> + ctx->final_response_sent = (resp->status >= 200)
>> + || (!strict && resp->status < 100);
>> + ctx->discard_body = ctx->final_response_sent &&
>> + (r->header_only ||
>> AP_STATUS_IS_HEADER_ONLY(resp->status));
>> +
>> + if (!ctx->tmpbb) {
>> + ctx->tmpbb = apr_brigade_create(r->pool,
>> c->bucket_alloc);
>> + }
>> + if (next != APR_BRIGADE_SENTINEL(b)) {
>> + apr_brigade_split_ex(b, next, ctx->tmpbb);
>> + }
>> +
>> + if (ctx->final_response_sent) {
>> + ap_h1_set_keepalive(r, resp);
>> +
>> + if (AP_STATUS_IS_HEADER_ONLY(resp->status)) {
>> + apr_table_unset(resp->headers,
>> "Transfer-Encoding");
>> + }
>> + else if (r->chunked) {
>> + apr_table_mergen(resp->headers,
>> "Transfer-Encoding", "chunked");
>> + apr_table_unset(resp->headers,
>> "Content-Length");
>> + }
>> + }
>> +
>> + /* kludge around broken browsers when indicated by
>> force-response-1.0
>> + */
>> + if (r->proto_num == HTTP_VERSION(1,0)
>> + && apr_table_get(r->subprocess_env,
>> "force-response-1.0")) {
>> + r->connection->keepalive = AP_CONN_CLOSE;
>> + proto = "HTTP/1.0";
>> + }
>> + h1_append_response_head(r, resp, proto, b);
>> + ap_h1_append_headers(b, r, resp->headers);
>> + ap_h1_terminate_header(b);
>> + apr_bucket_delete(e);
>> +
>> + if (ctx->final_response_sent && r->chunked) {
>> + /* We can't add this filter until we have already
>> sent the headers.
>> + * If we add it before this point, then the headers
>> will be chunked
>> + * as well, and that is just wrong.
>> + */
>> + rv = ap_pass_brigade(f->next, b);
>> + apr_brigade_cleanup(b);
>> + ap_log_rerror(APLOG_MARK, APLOG_TRACE2, rv, r,
>> + "ap_http1_response_out_filter passed
>> response"
>> + ", add CHUNK filter");
>> + if (APR_SUCCESS != rv) {
>> + apr_brigade_cleanup(ctx->tmpbb);
>> + goto cleanup;
>> + }
>> + ap_add_output_filter("CHUNK", NULL, r,
>> r->connection);
>> + }
>> +
>> + APR_BRIGADE_CONCAT(b, ctx->tmpbb);
>> +
>> + if (resp->status == 101) {
>
> Why using the literal 101 instead of HTTP_SWITCHING_PROTOCOLS?
>
>> + /* switched to another protocol, get out of the way
>> */
>> + AP_DEBUG_ASSERT(!r->chunked);
>> + ap_remove_output_filter(f);
>> + goto pass;
>> + }
>> + }
>> + }
>> + }
>> + else if (!ctx->final_response_sent && strict) {
>> + /* data buckets before seeing the final response are in error.
>> + */
>> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10390)
>> + "ap_http1_response_out_filter seeing data before
>> headers, %ld bytes ",
>> + (long)e->length);
>> + rv = AP_FILTER_ERROR;
>> + goto cleanup;
>> + }
>> + else if (ctx->discard_body) {
>> + apr_bucket_delete(e);
>> + }
>> + }
>> +
>> +pass:
>> + rv = ap_pass_brigade(f->next, b);
>> +cleanup:
>> + return rv;
>> +}
>> +
>
> Regards
>
> Rüdiger
>
