From: Ruediger Pluem Date: Tue, 8 Jan 2008 19:59:48 +0000 (+0000) Subject: Merge r609394, r609538, r609540 from trunk: X-Git-Tag: 2.2.8~10 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=56da3b4a788b6d7c85369dd327aead88018fdd83;p=thirdparty%2Fapache%2Fhttpd.git Merge r609394, r609538, r609540 from trunk: * Fix cases with non blocking reads from the ap_http_filter input filter where chunk size lines or empty lines after a chunk are read incomplete. This can lead to a corruption inside the dechunking algorithm. This patch has an issue with larger chunk-extensions which need to get thrown away since we ignore them anyway. PR: 19954, 41056 Tested by: niq * Optimize solution from r609394 and remove chunk-extensions restriction that was in r609394. * Optimize alignment. Submitted by: rpluem Reviewed by: rpluem, jim, niq git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.2.x@610119 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/CHANGES b/CHANGES index 32cae191e69..b0fa7990371 100644 --- a/CHANGES +++ b/CHANGES @@ -83,7 +83,7 @@ Changes with Apache 2.2.7 *) core: Fix broken chunk filtering that causes all non blocking reads to be converted into blocking reads. PR 19954, 41056. - [Jean-Frederic Clere, Jim Jagielski] + [Jean-Frederic Clere, Jim Jagielski, Ruediger Pluem, Nick Kew] *) mod_rewrite: Add the novary flag to RewriteCond. [Ruediger Pluem] diff --git a/STATUS b/STATUS index cec03e797f5..b4f2afd30bd 100644 --- a/STATUS +++ b/STATUS @@ -80,15 +80,6 @@ RELEASE SHOWSTOPPERS: PATCHES ACCEPTED TO BACKPORT FROM TRUNK: [ start all new proposals below, under PATCHES PROPOSED. ] - * core: Fix regression with non blocking reads from chunk input filter - Trunk version of patch: - http://svn.apache.org/viewvc?rev=609394&view=rev - http://svn.apache.org/viewvc?rev=609538&view=rev - http://svn.apache.org/viewvc?rev=609540&view=rev - Backport version for 2.2.x of patch: - Trunk version of patch works - +1: rpluem, jim, niq - PATCHES PROPOSED TO BACKPORT FROM TRUNK: [ New proposals should be added at the end of the list ] diff --git a/modules/http/http_filters.c b/modules/http/http_filters.c index e5d821cc250..aa658faf21e 100644 --- a/modules/http/http_filters.c +++ b/modules/http/http_filters.c @@ -68,8 +68,106 @@ typedef struct http_filter_ctx { BODY_CHUNK_PART } state; int eos_sent; + char chunk_ln[32]; + char *pos; + apr_off_t linesize; } http_ctx_t; +static apr_status_t get_remaining_chunk_line(http_ctx_t *ctx, + apr_bucket_brigade *b, + int linelimit) +{ + apr_status_t rv; + apr_off_t brigade_length; + apr_bucket *e; + const char *lineend; + apr_size_t len; + + /* + * As the brigade b should have been requested in mode AP_MODE_GETLINE + * all buckets in this brigade are already some type of memory + * buckets (due to the needed scanning for LF in mode AP_MODE_GETLINE) + * or META buckets. + */ + rv = apr_brigade_length(b, 0, &brigade_length); + if (rv != APR_SUCCESS) { + return rv; + } + /* Sanity check. Should never happen. See above. */ + if (brigade_length == -1) { + return APR_EGENERAL; + } + if (!brigade_length) { + return APR_EAGAIN; + } + ctx->linesize += brigade_length; + if (ctx->linesize > linelimit) { + return APR_ENOSPC; + } + /* + * As all buckets are already some type of memory buckets or META buckets + * (see above), we only need to check the last byte in the last data bucket. + */ + for (e = APR_BRIGADE_LAST(b); + e != APR_BRIGADE_SENTINEL(b); + e = APR_BUCKET_PREV(e)) { + + if (!APR_BUCKET_IS_METADATA(e)) { + break; + } + } + rv = apr_bucket_read(e, &lineend, &len, APR_BLOCK_READ); + if (rv != APR_SUCCESS) { + return rv; + } + if (lineend[len - 1] != APR_ASCII_LF) { + return APR_EAGAIN; + } + /* Line is complete. So reset ctx->linesize for next round. */ + ctx->linesize = 0; + return APR_SUCCESS; +} + +static apr_status_t get_chunk_line(http_ctx_t *ctx, apr_bucket_brigade *b, + int linelimit) +{ + apr_size_t len; + apr_status_t rv; + + len = sizeof(ctx->chunk_ln) - (ctx->pos - ctx->chunk_ln) - 1; + /* + * Check if there is space left in ctx->chunk_ln. If not, then either + * the chunk size is insane or we have chunk-extensions. Ignore both + * by discarding the remaining part of the line via + * get_remaining_chunk_line. Only bail out if the line is too long. + */ + if (len > 0) { + rv = apr_brigade_flatten(b, ctx->pos, &len); + if (rv != APR_SUCCESS) { + return rv; + } + ctx->pos += len; + ctx->linesize += len; + *(ctx->pos) = '\0'; + /* + * Check if we really got a full line. If yes the + * last char in the just read buffer must be LF. + * If not advance the buffer and return APR_EAGAIN. + * We do not start processing until we have the + * full line. + */ + if (ctx->pos[-1] != APR_ASCII_LF) { + /* Check if the remaining data in the brigade has the LF */ + return get_remaining_chunk_line(ctx, b, linelimit); + } + /* Line is complete. So reset ctx->pos for next round. */ + ctx->pos = ctx->chunk_ln; + return APR_SUCCESS; + } + return get_remaining_chunk_line(ctx, b, linelimit); +} + + /* This is the HTTP_INPUT filter for HTTP requests and responses from * proxied servers (mod_proxy). It handles chunked and content-length * bodies. This can only be inserted/used after the headers @@ -96,6 +194,8 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, ctx->remaining = 0; ctx->limit_used = 0; ctx->eos_sent = 0; + ctx->pos = ctx->chunk_ln; + ctx->linesize = 0; /* LimitRequestBody does not apply to proxied responses. * Consider implementing this check in its own filter. @@ -227,10 +327,7 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, /* We can't read the chunk until after sending 100 if required. */ if (ctx->state == BODY_CHUNK) { - char line[30]; apr_bucket_brigade *bb; - apr_size_t len = 30; - apr_off_t brigade_length; bb = apr_brigade_create(f->r->pool, f->c->bucket_alloc); @@ -246,20 +343,14 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, } if (rv == APR_SUCCESS) { - /* We have to check the length of the brigade we got back. - * We will not accept partial or blank lines. - */ - rv = apr_brigade_length(bb, 1, &brigade_length); - if (rv == APR_SUCCESS - && (!brigade_length || - brigade_length > f->r->server->limit_req_line)) { - rv = APR_ENOSPC; + rv = get_chunk_line(ctx, bb, f->r->server->limit_req_line); + if (APR_STATUS_IS_EAGAIN(rv)) { + apr_brigade_cleanup(bb); + ctx->state = BODY_CHUNK_PART; + return rv; } if (rv == APR_SUCCESS) { - rv = apr_brigade_flatten(bb, line, &len); - if (rv == APR_SUCCESS) { - ctx->remaining = get_chunk_size(line); - } + ctx->remaining = get_chunk_size(ctx->chunk_ln); } } apr_brigade_cleanup(bb); @@ -308,9 +399,7 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, case BODY_CHUNK: case BODY_CHUNK_PART: { - char line[30]; apr_bucket_brigade *bb; - apr_size_t len = 30; apr_status_t http_error = HTTP_REQUEST_ENTITY_TOO_LARGE; bb = apr_brigade_create(f->r->pool, f->c->bucket_alloc); @@ -319,11 +408,23 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, if (ctx->state == BODY_CHUNK) { rv = ap_get_brigade(f->next, bb, AP_MODE_GETLINE, block, 0); - apr_brigade_cleanup(bb); if (block == APR_NONBLOCK_READ && - (APR_STATUS_IS_EAGAIN(rv))) { + ( (rv == APR_SUCCESS && APR_BRIGADE_EMPTY(bb)) || + (APR_STATUS_IS_EAGAIN(rv)) )) { return APR_EAGAIN; } + /* + * We really don't care whats on this line. If it is RFC + * compliant it should be only \r\n. If there is more + * before we just ignore it as long as we do not get over + * the limit for request lines. + */ + rv = get_remaining_chunk_line(ctx, bb, + f->r->server->limit_req_line); + apr_brigade_cleanup(bb); + if (APR_STATUS_IS_EAGAIN(rv)) { + return rv; + } } else { rv = APR_SUCCESS; } @@ -341,15 +442,23 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, } ctx->state = BODY_CHUNK; if (rv == APR_SUCCESS) { - rv = apr_brigade_flatten(bb, line, &len); + rv = get_chunk_line(ctx, bb, f->r->server->limit_req_line); + if (APR_STATUS_IS_EAGAIN(rv)) { + ctx->state = BODY_CHUNK_PART; + apr_brigade_cleanup(bb); + return rv; + } if (rv == APR_SUCCESS) { - /* Wait a sec, that's a blank line! Oh no. */ - if (!len) { + /* + * Wait a sec, that's a blank line or it starts + * with an invalid character! Oh no. + */ + if (!apr_isxdigit(*(ctx->chunk_ln))) { rv = APR_EGENERAL; http_error = HTTP_SERVICE_UNAVAILABLE; } else { - ctx->remaining = get_chunk_size(line); + ctx->remaining = get_chunk_size(ctx->chunk_ln); } } }