From d755f54ffb7d2025b02d2cb1bdb3fed099362ec5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pokrywka?= Date: Thu, 28 Jan 2010 04:04:37 +0100 Subject: [PATCH 1/5] Set correct upload status when working with phusion passenger Passenger always sets status to 500 when exception occurs. This clashes with status set by LimitRequestBody handled in apache2's modules/http/http_filters.c:ap_http_filter Passenger's ext/apache2/Hooks.cpp:readRequestBodyFromApache fails on ap_get_brigade and throws RuntimeException. Fortunately status line is preserved, so correct error code can be parsed. --- mod_upload_progress.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/mod_upload_progress.c b/mod_upload_progress.c index c7007c1..b7b3b8c 100644 --- a/mod_upload_progress.c +++ b/mod_upload_progress.c @@ -466,10 +466,17 @@ static apr_status_t upload_progress_cleanup(void *data) { /* FIXME: this function should use locking because it modifies node data */ upload_progress_context_t *ctx = (upload_progress_context_t *)data; + int status; + if (ctx->node) { - if(ctx->r->status >= HTTP_BAD_REQUEST) - ctx->node->err_status = ctx->r->status; - ctx->node->expires = time(NULL) + 60; /*expires in 60s */ + /* error status rendered in status line is preferred because passenger + clobbers request_rec->status when exception occurs */ + status = ctx->r->status_line ? atoi(ctx->r->status_line) : 0; + if (!ap_is_HTTP_ERROR(status)) + status = ctx->r->status; + if (ap_is_HTTP_ERROR(status)) + ctx->node->err_status = status; + ctx->node->expires = time(NULL) + 60; /* expires in 60s */ ctx->node->done = 1; } return APR_SUCCESS; From 0eeea518320cf693c8ced2ea35ee6c646ced84ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pokrywka?= Date: Sat, 30 Jan 2010 10:29:47 +0100 Subject: [PATCH 2/5] Add Makefile to simplify building/installing module --- .gitignore | 1 + Makefile | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+) create mode 100644 Makefile diff --git a/.gitignore b/.gitignore index 6d36e6a..d9aea62 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,4 @@ *~ *.slo *.la +local-options.mk diff --git a/Makefile b/Makefile new file mode 100644 index 0000000..917c37d --- /dev/null +++ b/Makefile @@ -0,0 +1,32 @@ +-include local-options.mk + +ifndef APXS + APXS := $(shell which apxs2) +endif + +ifndef APXS + APXS := $(shell which apxs) +endif + +ifndef APXS + $(error No apxs found in path, set APXS = /path/to/apxs in local-options.mk) +endif + +all: local-shared-build + +local-shared-build: module + +module: .libs/mod_upload_progress.o + +.libs/mod_upload_progress.o: + $(APXS) -c $(CFLAGS) mod_upload_progress.c + +# install the shared object file into Apache +install: local-shared-build + $(APXS) -i $(CFLAGS) mod_upload_progress.la + +# cleanup +.PHONY: clean +clean: + -rm -f mod_upload_progress.o mod_upload_progress.lo mod_upload_progress.slo mod_upload_progress.la + -rm -rf .libs/ From 4241bb542db0b266de2f27ca2ca237ba84ac1410 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pokrywka?= Date: Sat, 30 Jan 2010 19:46:27 +0100 Subject: [PATCH 3/5] Add missing prerequisite to Makefile --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 917c37d..ab75a50 100644 --- a/Makefile +++ b/Makefile @@ -18,7 +18,7 @@ local-shared-build: module module: .libs/mod_upload_progress.o -.libs/mod_upload_progress.o: +.libs/mod_upload_progress.o: mod_upload_progress.c $(APXS) -c $(CFLAGS) mod_upload_progress.c # install the shared object file into Apache From 2e4f2c1f658aae1eb5afc7fbf20754e012c2d0f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pokrywka?= Date: Sun, 31 Jan 2010 01:04:29 +0100 Subject: [PATCH 4/5] Always save request status in progress cleanup function --- mod_upload_progress.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/mod_upload_progress.c b/mod_upload_progress.c index b7b3b8c..0ad1f48 100644 --- a/mod_upload_progress.c +++ b/mod_upload_progress.c @@ -219,6 +219,22 @@ void *upload_progress_config_create_server(apr_pool_t *p, server_rec *s) { return config; } +int read_request_status(request_rec *r) +{ + int status; + + if (r) { + /* error status rendered in status line is preferred because passenger + clobbers request_rec->status when exception occurs */ + status = r->status_line ? atoi(r->status_line) : 0; + if (!ap_is_HTTP_VALID_RESPONSE(status)) + status = r->status; + return status; + } else { + return 0; + } +} + static int track_upload_progress(ap_filter_t *f, apr_bucket_brigade *bb, ap_input_mode_t mode, apr_read_type_e block, apr_off_t readbytes) @@ -466,16 +482,9 @@ static apr_status_t upload_progress_cleanup(void *data) { /* FIXME: this function should use locking because it modifies node data */ upload_progress_context_t *ctx = (upload_progress_context_t *)data; - int status; if (ctx->node) { - /* error status rendered in status line is preferred because passenger - clobbers request_rec->status when exception occurs */ - status = ctx->r->status_line ? atoi(ctx->r->status_line) : 0; - if (!ap_is_HTTP_ERROR(status)) - status = ctx->r->status; - if (ap_is_HTTP_ERROR(status)) - ctx->node->err_status = status; + ctx->node->err_status = read_request_status(ctx->r); ctx->node->expires = time(NULL) + 60; /* expires in 60s */ ctx->node->done = 1; } From 6e27e3855e48ee4ae72c071b48a7692c30db74b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pokrywka?= Date: Sun, 31 Jan 2010 02:02:38 +0100 Subject: [PATCH 5/5] Make upload size limit violation immediately reported When LimitRequestBody is triggered, error message is sent to client immediately. Some clients (i.e. Firefox uploading file to iframe) does not read response until request is completed - whole file is uploaded anyway. This change makes upload progress report error to client early (before request is complete) so that client-side javascript can abort request. --- mod_upload_progress.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/mod_upload_progress.c b/mod_upload_progress.c index 0ad1f48..f94455e 100644 --- a/mod_upload_progress.c +++ b/mod_upload_progress.c @@ -243,31 +243,31 @@ static int track_upload_progress(ap_filter_t *f, apr_bucket_brigade *bb, upload_progress_node_t *node; ServerConfig* config = get_server_config(f->r); - if ((rv = ap_get_brigade(f->next, bb, mode, block, - readbytes)) != APR_SUCCESS) { - return rv; - } + rv = ap_get_brigade(f->next, bb, mode, block, readbytes); - apr_off_t length; - apr_brigade_length(bb, 1, &length); const char* id = get_progress_id(f->r); if (id == NULL) - return APR_SUCCESS; + return rv; CACHE_LOCK(); node = find_node(f->r, id); if (node != NULL) { - node->received += (int)length; - if (node->received > node->length) /* handle chunked tranfer */ - node->length = node->received; - int upload_time = time(NULL) - node->started_at; - if (upload_time > 0) { - node->speed = (int)(node->received / upload_time); - } + if (rv == APR_SUCCESS) { + apr_off_t length; + apr_brigade_length(bb, 1, &length); + node->received += (int)length; + if (node->received > node->length) /* handle chunked tranfer */ + node->length = node->received; + int upload_time = time(NULL) - node->started_at; + if (upload_time > 0) { + node->speed = (int)(node->received / upload_time); + } + } + node->err_status = read_request_status(f->r); } CACHE_UNLOCK(); - return APR_SUCCESS; + return rv; } const char *get_progress_id(request_rec *r) {