From 0a431ef862d309e2157ada8da485fb227cadd0dd Mon Sep 17 00:00:00 2001 From: Yann Ylavic Date: Tue, 10 Mar 2015 17:25:17 +0000 Subject: [PATCH] core, modules: like r1657897 but for core and other modules than mod_proxy. More uses of ap_map_http_request_error() and AP_FILTER_ERROR so that we never return an HTTP error status from a handler if some filter generated a response already. That is, from a handler, either ap_get_brigade() (an input filter) returned AP_FILTER_ERROR and we must forward it to ap_die(), or ap_pass_brigade() (an output filter) failed with any status and we must return AP_FILTER_ERROR in any case for ap_die() to determine whether a response is needed or not. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1665625 13f79535-47bb-0310-9956-ffa450edef68 --- modules/cache/mod_file_cache.c | 4 ++-- modules/cluster/mod_heartmonitor.c | 2 +- modules/dav/fs/repos.c | 2 +- modules/dav/main/mod_dav.c | 9 ++++++--- modules/filters/mod_reflector.c | 12 +++--------- modules/generators/mod_asis.c | 2 +- modules/generators/mod_cgi.c | 2 +- modules/generators/mod_cgid.c | 2 +- modules/http/http_filters.c | 12 +++++++++++- modules/proxy/mod_proxy_ajp.c | 11 +++-------- modules/ssl/ssl_engine_io.c | 2 +- modules/ssl/ssl_engine_kernel.c | 2 +- modules/test/mod_dialup.c | 6 +++--- server/core.c | 2 +- server/error_bucket.c | 3 +++ server/util_xml.c | 1 + 16 files changed, 40 insertions(+), 34 deletions(-) diff --git a/modules/cache/mod_file_cache.c b/modules/cache/mod_file_cache.c index d77b8d28a5..ca7b548356 100644 --- a/modules/cache/mod_file_cache.c +++ b/modules/cache/mod_file_cache.c @@ -283,7 +283,7 @@ static int mmap_handler(request_rec *r, a_file *file) APR_BRIGADE_INSERT_TAIL(bb, b); if (ap_pass_brigade(r->output_filters, bb) != APR_SUCCESS) - return HTTP_INTERNAL_SERVER_ERROR; + return AP_FILTER_ERROR; #endif return OK; } @@ -301,7 +301,7 @@ static int sendfile_handler(request_rec *r, a_file *file) APR_BRIGADE_INSERT_TAIL(bb, b); if (ap_pass_brigade(r->output_filters, bb) != APR_SUCCESS) - return HTTP_INTERNAL_SERVER_ERROR; + return AP_FILTER_ERROR; #endif return OK; } diff --git a/modules/cluster/mod_heartmonitor.c b/modules/cluster/mod_heartmonitor.c index b3d666ba22..784e5dd0fa 100644 --- a/modules/cluster/mod_heartmonitor.c +++ b/modules/cluster/mod_heartmonitor.c @@ -753,7 +753,7 @@ static int hm_handler(request_rec *r) input_brigade = apr_brigade_create(r->connection->pool, r->connection->bucket_alloc); status = ap_get_brigade(r->input_filters, input_brigade, AP_MODE_READBYTES, APR_BLOCK_READ, MAX_MSG_LEN); if (status != APR_SUCCESS) { - return HTTP_INTERNAL_SERVER_ERROR; + return ap_map_http_request_error(status, HTTP_BAD_REQUEST); } apr_brigade_flatten(input_brigade, buf, &len); diff --git a/modules/dav/fs/repos.c b/modules/dav/fs/repos.c index 950646eed8..6a5ff765f8 100644 --- a/modules/dav/fs/repos.c +++ b/modules/dav/fs/repos.c @@ -1105,7 +1105,7 @@ static dav_error * dav_fs_deliver(const dav_resource *resource, APR_BRIGADE_INSERT_TAIL(bb, bkt); if ((status = ap_pass_brigade(output, bb)) != APR_SUCCESS) { - return dav_new_error(pool, HTTP_FORBIDDEN, 0, status, + return dav_new_error(pool, AP_FILTER_ERROR, 0, status, "Could not write contents to filter."); } diff --git a/modules/dav/main/mod_dav.c b/modules/dav/main/mod_dav.c index 8eee84f823..1fff958954 100644 --- a/modules/dav/main/mod_dav.c +++ b/modules/dav/main/mod_dav.c @@ -582,6 +582,11 @@ static int dav_handle_err(request_rec *r, dav_error *err, /* log the errors */ dav_log_err(r, err, APLOG_ERR); + if (!ap_is_HTTP_VALID_RESPONSE(err->status)) { + /* we have responded already */ + return AP_FILTER_ERROR; + } + if (response == NULL) { dav_error *stackerr = err; @@ -1006,9 +1011,7 @@ static int dav_method_put(request_rec *r) "(URI: %s)", msg); } else { - /* XXX: should this actually be HTTP_BAD_REQUEST? */ - http_err = ap_map_http_request_error(rc, - HTTP_INTERNAL_SERVER_ERROR); + http_err = ap_map_http_request_error(rc, HTTP_BAD_REQUEST); msg = apr_psprintf(r->pool, "An error occurred while reading" " the request body (URI: %s)", msg); diff --git a/modules/filters/mod_reflector.c b/modules/filters/mod_reflector.c index 469df8e82b..961092d0ea 100644 --- a/modules/filters/mod_reflector.c +++ b/modules/filters/mod_reflector.c @@ -116,14 +116,8 @@ static int reflector_handler(request_rec * r) APR_BLOCK_READ, HUGE_STRING_LEN); if (status != APR_SUCCESS) { - if (status == AP_FILTER_ERROR) { - apr_brigade_destroy(bbin); - return status; - } - else { - apr_brigade_destroy(bbin); - return HTTP_BAD_REQUEST; - } + apr_brigade_destroy(bbin); + return ap_map_http_request_error(status, HTTP_BAD_REQUEST); } for (bucket = APR_BRIGADE_FIRST(bbin); @@ -160,7 +154,7 @@ static int reflector_handler(request_rec * r) ap_log_rerror(APLOG_MARK, APLOG_DEBUG, status, r, APLOGNO(01410) "reflector_handler: ap_pass_brigade returned %i", status); - return HTTP_INTERNAL_SERVER_ERROR; + return AP_FILTER_ERROR; } } diff --git a/modules/generators/mod_asis.c b/modules/generators/mod_asis.c index c947e30360..c2b651be2c 100644 --- a/modules/generators/mod_asis.c +++ b/modules/generators/mod_asis.c @@ -101,7 +101,7 @@ static int asis_handler(request_rec *r) if (rv != APR_SUCCESS) { ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01236) "mod_asis: ap_pass_brigade failed for file %s", r->filename); - return HTTP_INTERNAL_SERVER_ERROR; + return AP_FILTER_ERROR; } } else { diff --git a/modules/generators/mod_cgi.c b/modules/generators/mod_cgi.c index 0602867cdf..7c70119c94 100644 --- a/modules/generators/mod_cgi.c +++ b/modules/generators/mod_cgi.c @@ -857,7 +857,7 @@ static int cgi_handler(request_rec *r) } ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01225) "Error reading request entity data"); - return ap_map_http_request_error(rv, HTTP_INTERNAL_SERVER_ERROR); + return ap_map_http_request_error(rv, HTTP_BAD_REQUEST); } for (bucket = APR_BRIGADE_FIRST(bb); diff --git a/modules/generators/mod_cgid.c b/modules/generators/mod_cgid.c index 1950564028..50cc883067 100644 --- a/modules/generators/mod_cgid.c +++ b/modules/generators/mod_cgid.c @@ -1502,7 +1502,7 @@ static int cgid_handler(request_rec *r) } ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01270) "Error reading request entity data"); - return ap_map_http_request_error(rv, HTTP_INTERNAL_SERVER_ERROR); + return ap_map_http_request_error(rv, HTTP_BAD_REQUEST); } for (bucket = APR_BRIGADE_FIRST(bb); diff --git a/modules/http/http_filters.c b/modules/http/http_filters.c index c413e7108a..4bc7c82113 100644 --- a/modules/http/http_filters.c +++ b/modules/http/http_filters.c @@ -377,7 +377,10 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, e = apr_bucket_flush_create(f->c->bucket_alloc); APR_BRIGADE_INSERT_TAIL(bb, e); - ap_pass_brigade(f->c->output_filters, bb); + rv = ap_pass_brigade(f->c->output_filters, bb); + if (rv != APR_SUCCESS) { + return AP_FILTER_ERROR; + } } } } @@ -1597,6 +1600,13 @@ AP_DECLARE(long) ap_get_client_block(request_rec *r, char *buffer, /* We lose the failure code here. This is why ap_get_client_block should * not be used. */ + if (rv == AP_FILTER_ERROR) { + /* AP_FILTER_ERROR means a filter has responded already, + * we are DONE. + */ + apr_brigade_destroy(bb); + return -1; + } if (rv != APR_SUCCESS) { apr_bucket *e; diff --git a/modules/proxy/mod_proxy_ajp.c b/modules/proxy/mod_proxy_ajp.c index a0f3253c0e..bce7fac068 100644 --- a/modules/proxy/mod_proxy_ajp.c +++ b/modules/proxy/mod_proxy_ajp.c @@ -395,7 +395,7 @@ static int ap_proxy_ajp_request(apr_pool_t *p, request_rec *r, rv = HTTP_REQUEST_TIME_OUT; } else if (status == AP_FILTER_ERROR) { - data_sent = -1; + rv = AP_FILTER_ERROR; } output_failed = 1; break; @@ -611,12 +611,7 @@ static int ap_proxy_ajp_request(apr_pool_t *p, request_rec *r, "output: %i", backend_failed, output_failed); /* We had a failure: Close connection to backend */ conn->close = 1; - if (data_sent < 0) { - /* Return AP_FILTER_ERROR to let ap_die() handle the error */ - rv = AP_FILTER_ERROR; - data_sent = 0; - } - else if (data_sent) { + if (data_sent) { /* Return DONE to avoid error messages being added to the stream */ rv = DONE; } @@ -627,8 +622,8 @@ static int ap_proxy_ajp_request(apr_pool_t *p, request_rec *r, /* We had a failure: Close connection to backend */ conn->close = 1; backend_failed = 1; - /* Return DONE to avoid error messages being added to the stream */ if (data_sent) { + /* Return DONE to avoid error messages being added to the stream */ rv = DONE; } } diff --git a/modules/ssl/ssl_engine_io.c b/modules/ssl/ssl_engine_io.c index 4bd23a7e50..f0f970f827 100644 --- a/modules/ssl/ssl_engine_io.c +++ b/modules/ssl/ssl_engine_io.c @@ -1806,7 +1806,7 @@ int ssl_io_buffer_fill(request_rec *r, apr_size_t maxlen) if (rv) { ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(02015) "could not read request body for SSL buffer"); - return HTTP_INTERNAL_SERVER_ERROR; + return ap_map_http_request_error(rv, HTTP_INTERNAL_SERVER_ERROR); } /* Iterate through the returned brigade: setaside each bucket diff --git a/modules/ssl/ssl_engine_kernel.c b/modules/ssl/ssl_engine_kernel.c index 2fdd616cd5..0d5588db6d 100644 --- a/modules/ssl/ssl_engine_kernel.c +++ b/modules/ssl/ssl_engine_kernel.c @@ -133,7 +133,7 @@ int ssl_hook_ReadReq(request_rec *r) && (upgrade = apr_table_get(r->headers_in, "Upgrade")) != NULL && ap_find_token(r->pool, upgrade, "TLS/1.0")) { if (upgrade_connection(r)) { - return HTTP_INTERNAL_SERVER_ERROR; + return AP_FILTER_ERROR; } } diff --git a/modules/test/mod_dialup.c b/modules/test/mod_dialup.c index 460d31ad7a..a3abbc35f3 100644 --- a/modules/test/mod_dialup.c +++ b/modules/test/mod_dialup.c @@ -85,10 +85,10 @@ dialup_send_pulse(dialup_baton_t *db) apr_brigade_cleanup(db->tmpbb); - if (status != OK) { + if (status != APR_SUCCESS) { ap_log_rerror(APLOG_MARK, APLOG_ERR, status, db->r, APLOGNO(01867) "dialup: pulse: ap_pass_brigade failed:"); - return status; + return AP_FILTER_ERROR; } } @@ -121,7 +121,7 @@ dialup_callback(void *baton) return; } else { - ap_log_rerror(APLOG_MARK, APLOG_ERR, status, db->r, APLOGNO(01868) + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, db->r, APLOGNO(01868) "dialup: pulse returned: %d", status); db->r->status = HTTP_OK; ap_die(status, db->r); diff --git a/server/core.c b/server/core.c index 253438eacd..7928bbc98d 100644 --- a/server/core.c +++ b/server/core.c @@ -4675,7 +4675,7 @@ static int default_handler(request_rec *r) ap_log_rerror(APLOG_MARK, APLOG_DEBUG, status, r, APLOGNO(00133) "default_handler: ap_pass_brigade returned %i", status); - return HTTP_INTERNAL_SERVER_ERROR; + return AP_FILTER_ERROR; } } else { /* unusual method (not GET or POST) */ diff --git a/server/error_bucket.c b/server/error_bucket.c index 9c118e6144..52b55c35c4 100644 --- a/server/error_bucket.c +++ b/server/error_bucket.c @@ -61,6 +61,9 @@ AP_DECLARE(apr_bucket *) ap_bucket_error_create(int error, const char *buf, APR_BUCKET_INIT(b); b->free = apr_bucket_free; b->list = list; + if (!ap_is_HTTP_VALID_RESPONSE(error)) { + error = HTTP_INTERNAL_SERVER_ERROR; + } return ap_bucket_error_make(b, error, buf, p); } diff --git a/server/util_xml.c b/server/util_xml.c index 26f1c6e842..4845194656 100644 --- a/server/util_xml.c +++ b/server/util_xml.c @@ -59,6 +59,7 @@ AP_DECLARE(int) ap_xml_parse_input(request_rec * r, apr_xml_doc **pdoc) READ_BLOCKSIZE); if (status != APR_SUCCESS) { + result = ap_map_http_request_error(status, HTTP_BAD_REQUEST); goto read_error; } -- 2.40.0