Added mod_log_config changes; I used "O" and "I" for the format letters.
Not sure if there are letters that might be more preferred.
On Fri, May 9, 2014 at 11:01 AM, Edward Lu <[email protected]> wrote:
> Here's a 2.2.x backport of Yann's patch with a directive, MergeTrailers,
> added in to opt-in to the old behavior. By default, it doesn't merge the
> trailers. I'm working on adding the capability to mod_log_config to log the
> trailers, but I figured I'd post this patch first to get some review.
>
> - Ed
>
>
> On Tue, May 6, 2014 at 7:45 PM, Eric Covener <[email protected]> wrote:
>
>> On Tue, May 6, 2014 at 6:44 PM, Yann Ylavic <[email protected]> wrote:
>> > 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.
>>
>> I think I could be on board for that plan. Any others?
>>
>> >
>> > 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?
>>
>> For 2.2, I would personally want to have the ability to restore the
>> old merging behavior before any release. Joe drew his line there as
>> well.
>>
>> But I think Ed Lu is lurking and can take that last bit on.
>>
>> --
>> Eric Covener
>> [email protected]
>>
>
>
Index: include/http_core.h
===================================================================
--- include/http_core.h (revision 1593540)
+++ include/http_core.h (working copy)
@@ -613,6 +613,10 @@
#define AP_TRACE_ENABLE 1
#define AP_TRACE_EXTENDED 2
int trace_enable;
+#define AP_MERGE_TRAILERS_UNSET 0
+#define AP_MERGE_TRAILERS_ENABLE 1
+#define AP_MERGE_TRAILERS_DISABLE 2
+ int merge_trailers;
} core_server_config;
Index: include/httpd.h
===================================================================
--- include/httpd.h (revision 1593540)
+++ include/httpd.h (working copy)
@@ -1006,6 +1006,11 @@
* record to improve 64bit alignment the next time we need to break
* binary compatibility for some other reason.
*/
+
+ /** MIME trailer environment from the request */
+ apr_table_t *trailers_in;
+ /** MIME trailer environment from the response */
+ apr_table_t *trailers_out;
};
/**
Index: modules/http/http_filters.c
===================================================================
--- modules/http/http_filters.c (revision 1593540)
+++ modules/http/http_filters.c (working copy)
@@ -215,6 +215,7 @@
ap_input_mode_t mode, apr_read_type_e block,
apr_off_t readbytes)
{
+ core_server_config *conf;
apr_bucket *e;
http_ctx_t *ctx = f->ctx;
apr_status_t rv;
@@ -222,6 +223,9 @@
int http_error = HTTP_REQUEST_ENTITY_TOO_LARGE;
apr_bucket_brigade *bb;
+ conf = (core_server_config *)
+ ap_get_module_config(f->r->server->module_config, &core_module);
+
/* just get out of the way of things we don't want. */
if (mode != AP_MODE_READBYTES && mode != AP_MODE_GETLINE) {
return ap_get_brigade(f->next, b, mode, block, readbytes);
@@ -403,13 +407,50 @@
}
if (!ctx->remaining) {
- /* Handle trailers by calling ap_get_mime_headers again! */
+ 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");
+
+ /* By default, don't merge in the trailers
+ * (treat UNSET as DISABLE) */
+ if (conf->merge_trailers != AP_MERGE_TRAILERS_ENABLE) {
+ f->r->status = HTTP_OK;
+ f->r->headers_in = f->r->trailers_in;
+ apr_table_clear(f->r->headers_in);
+ }
+
ctx->state = BODY_NONE;
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,
+ "Error while reading HTTP trailer: %i%s%s",
+ f->r->status, error_notes ? ": " : "",
+ error_notes ? error_notes : "");
+ rv = APR_EINVAL;
+ }
+
+ if (conf->merge_trailers != AP_MERGE_TRAILERS_ENABLE) {
+ 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;
}
}
}
Index: modules/http/http_request.c
===================================================================
--- modules/http/http_request.c (revision 1593540)
+++ modules/http/http_request.c (working copy)
@@ -384,8 +384,10 @@
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);
new->err_headers_out = r->err_headers_out;
+ new->trailers_out = r->trailers_out;
new->subprocess_env = rename_original_env(r->pool, r->subprocess_env);
new->notes = apr_table_make(r->pool, 5);
@@ -495,6 +497,8 @@
r->headers_out);
r->err_headers_out = apr_table_overlay(r->pool, rr->err_headers_out,
r->err_headers_out);
+ r->err_headers_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/loggers/mod_log_config.c
===================================================================
--- modules/loggers/mod_log_config.c (revision 1593540)
+++ modules/loggers/mod_log_config.c (working copy)
@@ -412,6 +412,12 @@
return ap_escape_logitem(r->pool, apr_table_get(r->headers_in, a));
}
+static const char *log_trailer_in(request_rec *r, char *a)
+{
+ return ap_escape_logitem(r->pool, apr_table_get(r->trailers_in, a));
+}
+
+
static APR_INLINE char *find_multiple_headers(apr_pool_t *pool,
const apr_table_t *table,
const char *key)
@@ -495,6 +501,11 @@
return ap_escape_logitem(r->pool, cp);
}
+static const char *log_trailer_out(request_rec *r, char *a)
+{
+ return ap_escape_logitem(r->pool, apr_table_get(r->trailers_out, a));
+}
+
static const char *log_note(request_rec *r, char *a)
{
return ap_escape_logitem(r->pool, apr_table_get(r->notes, a));
@@ -1540,6 +1551,8 @@
log_pfn_register(p, "B", log_bytes_sent, 0);
log_pfn_register(p, "i", log_header_in, 0);
log_pfn_register(p, "o", log_header_out, 0);
+ log_pfn_register(p, "I", log_trailer_in, 0);
+ log_pfn_register(p, "O", log_trailer_out, 0);
log_pfn_register(p, "n", log_note, 0);
log_pfn_register(p, "e", log_env_var, 0);
log_pfn_register(p, "V", log_server_name, 0);
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c (revision 1593540)
+++ modules/proxy/mod_proxy_http.c (working copy)
@@ -1229,6 +1229,7 @@
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;
/*
@@ -1355,6 +1356,14 @@
#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
apr_status_t ap_proxy_http_process_response(apr_pool_t * p, request_rec *r,
proxy_conn_rec *backend,
@@ -1809,6 +1818,12 @@
/* 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: server/core.c
===================================================================
--- server/core.c (revision 1593540)
+++ server/core.c (working copy)
@@ -542,6 +542,10 @@
? virt->trace_enable
: base->trace_enable;
+ conf->merge_trailers = (virt->trace_enable != AP_MERGE_TRAILERS_UNSET)
+ ? virt->merge_trailers
+ : base->merge_trailers;
+
return conf;
}
@@ -3295,6 +3299,16 @@
return NULL;
}
+static const char *set_merge_trailers(cmd_parms *cmd, void *dummy, int arg)
+{
+ core_server_config *conf = ap_get_module_config(cmd->server->module_config,
+ &core_module);
+ conf->merge_trailers = (arg ? AP_MERGE_TRAILERS_ENABLE :
+ AP_MERGE_TRAILERS_DISABLE);
+
+ return NULL;
+}
+
/* Note --- ErrorDocument will now work from .htaccess files.
* The AllowOverride of Fileinfo allows webmasters to turn it off
*/
@@ -3530,6 +3544,8 @@
#endif
AP_INIT_TAKE1("TraceEnable", set_trace_enable, NULL, RSRC_CONF,
"'on' (default), 'off' or 'extended' to trace request body content"),
+AP_INIT_FLAG("MergeTrailers", set_merge_trailers, NULL, RSRC_CONF,
+ "merge request trailers into request headers or not"),
#ifdef SUEXEC_BIN
AP_INIT_FLAG("Suexec", unixd_set_suexec, NULL, RSRC_CONF,
"Enable or disable suEXEC support"),
@@ -3632,7 +3648,6 @@
static int do_nothing(request_rec *r) { return OK; }
-
static int core_override_type(request_rec *r)
{
core_dir_config *conf =
Index: server/protocol.c
===================================================================
--- server/protocol.c (revision 1593540)
+++ server/protocol.c (working copy)
@@ -887,9 +887,11 @@
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);
@@ -1141,7 +1143,8 @@
rnew->status = HTTP_OK;
- rnew->headers_in = apr_table_copy(rnew->pool, r->headers_in);
+ 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
@@ -1153,6 +1156,7 @@
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;
