On Tue, May 6, 2014 at 1:41 PM, Eric Covener <[email protected]> wrote:
> I'd really like to start chucking trailers, but the scale of these
> patches really frightens me for 2.4 and 2.2 and accompanying what will
> likely be a security roll-up.
Understood.
>
> Is anyone sitting on a more tactical version of the fix?
Well, maybe I should stop proposing something here but maybe the
attached patch can help.
I think it's a "tactical" version of the previous one, in that it does
*not* touch ap_get_mime_headers*() (nor ap_rgetline*()), but
saves/restores the data (request's fields, notes) modified by the
function when called from ap_http_filter().
It could be a way for 2.2.x, but I don't think it's enough for trunk
or even 2.4.x, because this implementation still breaks non-blocking
mode (hence event), which is (also) why the core functions were
modified in the previous patch.
This patch (still) does not propose to merge splitted trailers into
headers (for those broken by the change), but when (and why) would we
do that?
Index: server/protocol.c
===================================================================
--- server/protocol.c (revision 1592855)
+++ server/protocol.c (working copy)
@@ -1010,9 +1010,11 @@ request_rec *ap_read_request(conn_rec *conn)
r->allowed_methods = ap_make_method_list(p, 2);
r->headers_in = apr_table_make(r->pool, 25);
+ r->trailers_in = apr_table_make(r->pool, 5);
r->subprocess_env = apr_table_make(r->pool, 25);
r->headers_out = apr_table_make(r->pool, 12);
r->err_headers_out = apr_table_make(r->pool, 5);
+ r->trailers_out = apr_table_make(r->pool, 5);
r->notes = apr_table_make(r->pool, 5);
r->request_config = ap_create_request_config(r->pool);
@@ -1289,6 +1291,7 @@ AP_DECLARE(void) ap_set_sub_req_protocol(request_r
rnew->status = HTTP_OK;
rnew->headers_in = apr_table_copy(rnew->pool, r->headers_in);
+ rnew->trailers_in = apr_table_copy(rnew->pool, r->trailers_in);
/* did the original request have a body? (e.g. POST w/SSI tags)
* if so, make sure the subrequest doesn't inherit body headers
@@ -1300,6 +1303,7 @@ AP_DECLARE(void) ap_set_sub_req_protocol(request_r
rnew->subprocess_env = apr_table_copy(rnew->pool, r->subprocess_env);
rnew->headers_out = apr_table_make(rnew->pool, 5);
rnew->err_headers_out = apr_table_make(rnew->pool, 5);
+ rnew->trailers_out = apr_table_make(rnew->pool, 5);
rnew->notes = apr_table_make(rnew->pool, 5);
rnew->expecting_100 = r->expecting_100;
Index: modules/http/http_filters.c
===================================================================
--- modules/http/http_filters.c (revision 1592855)
+++ modules/http/http_filters.c (working copy)
@@ -396,11 +396,41 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bu
again = 1; /* come around again */
if (ctx->state == BODY_CHUNK_TRAILER) {
+ int saved_status = f->r->status;
+ apr_table_t *saved_headers_in = f->r->headers_in;
+ const char *saved_error_notes = apr_table_get(f->r->notes,
+ "error-notes");
+
+ f->r->status = HTTP_OK;
+ f->r->headers_in = f->r->trailers_in;
+ apr_table_clear(f->r->headers_in);
ap_get_mime_headers(f->r);
- e = apr_bucket_eos_create(f->c->bucket_alloc);
- APR_BRIGADE_INSERT_TAIL(b, e);
- ctx->eos_sent = 1;
- return APR_SUCCESS;
+ if (f->r->status == HTTP_OK) {
+ e = apr_bucket_eos_create(f->c->bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(b, e);
+ ctx->eos_sent = 1;
+ rv = APR_SUCCESS;
+ }
+ else {
+ const char *error_notes = apr_table_get(f->r->notes,
+ "error-notes");
+ ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO()
+ "Error while reading HTTP trailer: %i%s%s",
+ f->r->status, error_notes ? ": " : "",
+ error_notes ? error_notes : "");
+ rv = APR_EINVAL;
+ }
+
+ f->r->status = saved_status;
+ f->r->headers_in = saved_headers_in;
+ if (saved_error_notes) {
+ apr_table_setn(f->r->notes, "error-notes",
+ saved_error_notes);
+ }
+ else {
+ apr_table_unset(f->r->notes, "error-notes");
+ }
+ return rv;
}
break;
Index: modules/http/http_request.c
===================================================================
--- modules/http/http_request.c (revision 1592855)
+++ modules/http/http_request.c (working copy)
@@ -463,6 +463,7 @@ static request_rec *internal_internal_redirect(con
new->main = r->main;
new->headers_in = r->headers_in;
+ new->trailers_in = r->trailers_in;
new->headers_out = apr_table_make(r->pool, 12);
if (ap_is_HTTP_REDIRECT(new->status)) {
const char *location = apr_table_get(r->headers_out, "Location");
@@ -470,6 +471,7 @@ static request_rec *internal_internal_redirect(con
apr_table_setn(new->headers_out, "Location", location);
}
new->err_headers_out = r->err_headers_out;
+ new->trailers_out = apr_table_make(r->pool, 5);
new->subprocess_env = rename_original_env(r->pool, r->subprocess_env);
new->notes = apr_table_make(r->pool, 5);
@@ -583,6 +585,8 @@ AP_DECLARE(void) ap_internal_fast_redirect(request
r->headers_out);
r->err_headers_out = apr_table_overlay(r->pool, rr->err_headers_out,
r->err_headers_out);
+ r->trailers_out = apr_table_overlay(r->pool, rr->trailers_out,
+ r->trailers_out);
r->subprocess_env = apr_table_overlay(r->pool, rr->subprocess_env,
r->subprocess_env);
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c (revision 1592855)
+++ modules/proxy/mod_proxy_http.c (working copy)
@@ -1004,9 +1004,11 @@ static request_rec *make_fake_req(conn_rec *c, req
rp->status = HTTP_OK;
rp->headers_in = apr_table_make(pool, 50);
+ rp->trailers_in = apr_table_make(pool, 5);
rp->subprocess_env = apr_table_make(pool, 50);
rp->headers_out = apr_table_make(pool, 12);
rp->err_headers_out = apr_table_make(pool, 5);
+ rp->trailers_out = apr_table_make(pool, 5);
rp->notes = apr_table_make(pool, 5);
rp->server = r->server;
@@ -1086,6 +1088,7 @@ static void ap_proxy_read_headers(request_rec *r,
psc = (proxy_server_conf *) ap_get_module_config(sconf, &proxy_module);
r->headers_out = apr_table_make(r->pool, 20);
+ r->trailers_out = apr_table_make(r->pool, 5);
*pread_len = 0;
/*
@@ -1216,6 +1219,14 @@ apr_status_t ap_proxygetline(apr_bucket_brigade *b
#define AP_MAX_INTERIM_RESPONSES 10
#endif
+static int add_trailers(void *data, const char *key, const char *val)
+{
+ if (val) {
+ apr_table_add((apr_table_t*)data, key, val);
+ }
+ return 1;
+}
+
static
int ap_proxy_http_process_response(apr_pool_t * p, request_rec *r,
proxy_conn_rec **backend_ptr, proxy_worker *worker,
@@ -1733,6 +1744,12 @@ int ap_proxy_http_process_response(apr_pool_t * p,
/* next time try a non-blocking read */
mode = APR_NONBLOCK_READ;
+ if (!apr_is_empty_table(backend->r->trailers_in)) {
+ apr_table_do(add_trailers, r->trailers_out,
+ backend->r->trailers_in, NULL);
+ apr_table_clear(backend->r->trailers_in);
+ }
+
apr_brigade_length(bb, 0, &readbytes);
backend->worker->s->read += readbytes;
#if DEBUGGING
Index: include/httpd.h
===================================================================
--- include/httpd.h (revision 1592855)
+++ include/httpd.h (working copy)
@@ -1039,6 +1039,11 @@ struct request_rec {
*/
apr_sockaddr_t *useragent_addr;
char *useragent_ip;
+
+ /** MIME trailer environment from the request */
+ apr_table_t *trailers_in;
+ /** MIME trailer environment for the response */
+ apr_table_t *trailers_out;
};
/**
