You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by je...@apache.org on 2002/05/06 09:43:40 UTC
cvs commit: httpd-2.0/modules/http http_protocol.c
jerenkrantz 02/05/06 00:43:40
Modified: . CHANGES STATUS
include httpd.h
modules/http http_protocol.c
Log:
Rewrite ap_byterange_filter so that it can work with data that does not
have a predetermined C-L - such as data that passes through mod_include.
Previously, these requests would generate 416 since when the byterange
filter ran, r->clength would be 0. r->clength is only guaranteed to
be valid after C-L filter is run, but we need C-L to run after us so
that our data can have a proper C-L returned. So, we need to rearrange
the code so that we can deal with this case.
Highlights:
- Remove r->boundary since it is possible to have this self-contained in
boundary's ctx. (May require MMN bump?)
- Remove call to parse_byteranges in ap_set_byterange since this would
wrongly return -1 for dynamic responses. We have to wait until we
see EOS to call parse_byteranges.
- Move bound_head computation inside the num_parts == 2 check.
- Change a NULL brigade check to APR_BRIGADE_EMPTY
- Move the 416 error return to after we've run through all ranges and
found none of them to be valid.
Revision Changes Path
1.760 +3 -0 httpd-2.0/CHANGES
Index: CHANGES
===================================================================
RCS file: /home/cvs/httpd-2.0/CHANGES,v
retrieving revision 1.759
retrieving revision 1.760
diff -u -r1.759 -r1.760
--- CHANGES 6 May 2002 03:10:22 -0000 1.759
+++ CHANGES 6 May 2002 07:43:39 -0000 1.760
@@ -1,5 +1,8 @@
Changes with Apache 2.0.37
+ *) Fix byterange requests from returning 416 when using dynamic data
+ (such as filters like mod_include). [Justin Erenkrantz]
+
*) Allow mod_rewrite's set of "int:" internal RewriteMap functions
to be extended by third-party modules via an optional function.
[Tahiry Ramanamampanoharana <no...@hotmail.com>, Cliff Woolley]
1.610 +1 -5 httpd-2.0/STATUS
Index: STATUS
===================================================================
RCS file: /home/cvs/httpd-2.0/STATUS,v
retrieving revision 1.609
retrieving revision 1.610
diff -u -r1.609 -r1.610
--- STATUS 3 May 2002 05:50:40 -0000 1.609
+++ STATUS 6 May 2002 07:43:39 -0000 1.610
@@ -1,5 +1,5 @@
APACHE 2.0 STATUS: -*-text-*-
-Last modified at [$Date: 2002/05/03 05:50:40 $]
+Last modified at [$Date: 2002/05/06 07:43:39 $]
Release:
@@ -96,10 +96,6 @@
close them all as soon as the listener thread receives the
signal to shutdown.
Status: Aaron volunteers
-
- * Incorrect Content-Range headers or invalid 416 HTTP responses
- if a filter such as INCLUDES changes the content length. It may
- happen only when there are multiple output brigades.
* --enable-mods-shared="foo1 foo2" is busted on Darwin. Pier
posted a patch (Message-ID: <B8...@betaversion.org>).
1.183 +0 -2 httpd-2.0/include/httpd.h
Index: httpd.h
===================================================================
RCS file: /home/cvs/httpd-2.0/include/httpd.h,v
retrieving revision 1.182
retrieving revision 1.183
diff -u -r1.182 -r1.183
--- httpd.h 29 Mar 2002 08:17:19 -0000 1.182
+++ httpd.h 6 May 2002 07:43:40 -0000 1.183
@@ -795,8 +795,6 @@
/** sending chunked transfer-coding */
int chunked;
- /** multipart/byteranges boundary */
- const char *boundary;
/** The Range: header */
const char *range;
/** The "real" content length */
1.416 +43 -64 httpd-2.0/modules/http/http_protocol.c
Index: http_protocol.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/http_protocol.c,v
retrieving revision 1.415
retrieving revision 1.416
diff -u -r1.415 -r1.416
--- http_protocol.c 23 Apr 2002 22:15:08 -0000 1.415
+++ http_protocol.c 6 May 2002 07:43:40 -0000 1.416
@@ -2600,7 +2600,8 @@
typedef struct byterange_ctx {
apr_bucket_brigade *bb;
int num_ranges;
- const char *orig_ct;
+ char *boundary;
+ char *bound_head;
} byterange_ctx;
/*
@@ -2634,7 +2635,6 @@
apr_off_t range_start;
apr_off_t range_end;
char *current;
- char *bound_head;
apr_off_t bb_length;
apr_off_t clength = 0;
apr_status_t rv;
@@ -2643,16 +2643,7 @@
if (!ctx) {
int num_ranges = ap_set_byterange(r);
- if (num_ranges == -1) {
- ap_remove_output_filter(f);
- bsend = apr_brigade_create(r->pool, c->bucket_alloc);
- e = ap_bucket_error_create(HTTP_RANGE_NOT_SATISFIABLE, NULL,
- r->pool, c->bucket_alloc);
- APR_BRIGADE_INSERT_TAIL(bsend, e);
- e = apr_bucket_eos_create(c->bucket_alloc);
- APR_BRIGADE_INSERT_TAIL(bsend, e);
- return ap_pass_brigade(f->next, bsend);
- }
+ /* We have nothing to do, get out of the way. */
if (num_ranges == 0) {
ap_remove_output_filter(f);
return ap_pass_brigade(f->next, bb);
@@ -2660,17 +2651,28 @@
ctx = f->ctx = apr_pcalloc(r->pool, sizeof(*ctx));
ctx->num_ranges = num_ranges;
+ /* create a brigade in case we never call ap_save_brigade() */
+ ctx->bb = apr_brigade_create(r->pool, c->bucket_alloc);
+
+ if (ctx->num_ranges > 1) {
+ /* Is ap_make_content_type required here? */
+ const char *orig_ct = ap_make_content_type(r, r->content_type);
+ ctx->boundary = apr_psprintf(r->pool, "%qx%lx",
+ r->request_time, (long) getpid());
- if (num_ranges > 1) {
- ctx->orig_ct = r->content_type;
ap_set_content_type(r, apr_pstrcat(r->pool, "multipart",
use_range_x(r) ? "/x-" : "/",
"byteranges; boundary=",
- r->boundary, NULL));
- }
+ ctx->boundary, NULL));
- /* create a brigade in case we never call ap_save_brigade() */
- ctx->bb = apr_brigade_create(r->pool, c->bucket_alloc);
+ ctx->bound_head = apr_pstrcat(r->pool,
+ CRLF "--", ctx->boundary,
+ CRLF "Content-type: ",
+ orig_ct,
+ CRLF "Content-range: bytes ",
+ NULL);
+ ap_xlate_proto_to_ascii(ctx->bound_head, strlen(ctx->bound_head));
+ }
}
/* We can't actually deal with byte-ranges until we have the whole brigade
@@ -2682,28 +2684,19 @@
return APR_SUCCESS;
}
- /* compute this once (it is an invariant) */
- bound_head = apr_pstrcat(r->pool,
- CRLF "--", r->boundary,
- CRLF "Content-type: ",
- ap_make_content_type(r, ctx->orig_ct),
- CRLF "Content-range: bytes ",
- NULL);
- ap_xlate_proto_to_ascii(bound_head, strlen(bound_head));
-
/* If we have a saved brigade from a previous run, concat the passed
* brigade with our saved brigade. Otherwise just continue.
*/
- if (ctx->bb) {
+ if (!APR_BRIGADE_EMPTY(ctx->bb)) {
APR_BRIGADE_CONCAT(ctx->bb, bb);
bb = ctx->bb;
- ctx->bb = NULL; /* ### strictly necessary? call brigade_destroy? */
}
+ apr_brigade_destroy(ctx->bb);
/* It is possible that we won't have a content length yet, so we have to
* compute the length before we can actually do the byterange work.
*/
- (void) apr_brigade_length(bb, 1, &bb_length);
+ apr_brigade_length(bb, 1, &bb_length);
clength = (apr_off_t)bb_length;
/* this brigade holds what we will be sending */
@@ -2735,10 +2728,18 @@
found = 1;
- if (ctx->num_ranges > 1) {
+ /* For single range requests, we must produce Content-Range header.
+ * Otherwise, we need to produce the multipart boundaries.
+ */
+ if (ctx->num_ranges == 1) {
+ apr_table_setn(r->headers_out, "Content-Range",
+ apr_psprintf(r->pool, "bytes " BYTERANGE_FMT,
+ range_start, range_end, clength));
+ }
+ else {
char *ts;
- e = apr_bucket_pool_create(bound_head, strlen(bound_head),
+ e = apr_bucket_pool_create(ctx->bound_head, strlen(ctx->bound_head),
r->pool, c->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(bsend, e);
@@ -2774,14 +2775,20 @@
if (found == 0) {
ap_remove_output_filter(f);
r->status = HTTP_OK;
- return HTTP_RANGE_NOT_SATISFIABLE;
+ /* bsend is assumed to be empty if we get here. */
+ e = ap_bucket_error_create(HTTP_RANGE_NOT_SATISFIABLE, NULL,
+ r->pool, c->bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(bsend, e);
+ e = apr_bucket_eos_create(c->bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(bsend, e);
+ return ap_pass_brigade(f->next, bsend);
}
if (ctx->num_ranges > 1) {
char *end;
/* add the final boundary */
- end = apr_pstrcat(r->pool, CRLF "--", r->boundary, "--" CRLF, NULL);
+ end = apr_pstrcat(r->pool, CRLF "--", ctx->boundary, "--" CRLF, NULL);
ap_xlate_proto_to_ascii(end, strlen(end));
e = apr_bucket_pool_create(end, strlen(end), r->pool, c->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(bsend, e);
@@ -2790,7 +2797,7 @@
e = apr_bucket_eos_create(c->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(bsend, e);
- /* we're done with the original content */
+ /* we're done with the original content - all of our data is in bsend. */
apr_brigade_destroy(bb);
/* send our multipart output */
@@ -2803,8 +2810,6 @@
const char *if_range;
const char *match;
const char *ct;
- apr_off_t range_start;
- apr_off_t range_end;
int num_ranges;
if (r->assbackwards) {
@@ -2858,39 +2863,13 @@
}
}
- /* would be nice to pick this up from f->ctx */
- ct = ap_make_content_type(r, r->content_type);
-
if (!ap_strchr_c(range, ',')) {
- int rv;
- /* A single range */
-
- /* parse_byterange() modifies the contents, so make a copy */
- if ((rv = parse_byterange(apr_pstrdup(r->pool, range + 6), r->clength,
- &range_start, &range_end)) <= 0) {
- return rv;
- }
- apr_table_setn(r->headers_out, "Content-Range",
- apr_psprintf(r->pool, "bytes " BYTERANGE_FMT,
- range_start, range_end, r->clength));
- apr_table_setn(r->headers_out, "Content-Type", ct);
-
+ /* a single range */
num_ranges = 1;
}
else {
/* a multiple range */
-
num_ranges = 2;
-
- /* ### it would be nice if r->boundary was in f->ctx */
- r->boundary = apr_psprintf(r->pool, "%qx%lx",
- r->request_time, (long) getpid());
-
- apr_table_setn(r->headers_out, "Content-Type",
- apr_pstrcat(r->pool,
- "multipart", use_range_x(r) ? "/x-" : "/",
- "byteranges; boundary=", r->boundary,
- NULL));
}
r->status = HTTP_PARTIAL_CONTENT;
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
Posted by Justin Erenkrantz <je...@apache.org>.
On Tue, May 07, 2002 at 12:50:35PM -0700, Greg Stein wrote:
> Woah... now that I'm seeing this in context, the MMN bump is awfully
> gratuitous!
My take is that if there were no MMN, this change is correct. It
removes a public field and makes it only local to where it is
needed. I don't think that r->boundary isn't necessary anywhere
outside of the byterange filter.
I'm not sure I like the idea of having unused fields. -- justin
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
Posted by Greg Stein <gs...@lyra.org>.
Woah... now that I'm seeing this in context, the MMN bump is awfully
gratuitous!
Personally, I'd suggest that we back out this particular change. Or at a
minimum, list its resolution as a showstopper. That is: at release time,
resolve whether we want to shift this back, or that other changes have
*demanded* an MMN bump (thus, this one can just fall in there).
Granted: I can't see anybody referring to this stupid field, but the change
in the offsets for fields is a definite problem.
Cheers,
-g
On Mon, May 06, 2002 at 07:43:40AM -0000, jerenkrantz@apache.org wrote:
>...
> Highlights:
> - Remove r->boundary since it is possible to have this self-contained in
> boundary's ctx. (May require MMN bump?)
>...
> +++ httpd.h 6 May 2002 07:43:40 -0000 1.183
> @@ -795,8 +795,6 @@
>
> /** sending chunked transfer-coding */
> int chunked;
> - /** multipart/byteranges boundary */
> - const char *boundary;
> /** The Range: header */
> const char *range;
> /** The "real" content length */
>...
> +++ http_protocol.c 6 May 2002 07:43:40 -0000 1.416
> @@ -2600,7 +2600,8 @@
> typedef struct byterange_ctx {
> apr_bucket_brigade *bb;
> int num_ranges;
> - const char *orig_ct;
> + char *boundary;
> + char *bound_head;
> } byterange_ctx;
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
Posted by Justin Erenkrantz <je...@apache.org>.
On Mon, May 06, 2002 at 07:43:40AM -0000, jerenkrantz@apache.org wrote:
> - Remove r->boundary since it is possible to have this self-contained in
> boundary's ctx. (May require MMN bump?)
I believe removing a field from request_rec requires a MMN bump. Am I
right? -- justin
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
Posted by Greg Ames <gr...@apache.org>.
jerenkrantz@apache.org wrote:
>
> jerenkrantz 02/05/06 00:43:40
>
> Modified: . CHANGES STATUS
> include httpd.h
> modules/http http_protocol.c
> Log:
> Rewrite ap_byterange_filter so that it can work with data that does not
> have a predetermined C-L - such as data that passes through mod_include.
just tried this with my SSI/Range: test cases...I can't break it any more.
Nice job, Justin!
Greg
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
Posted by Cliff Woolley <jw...@virginia.edu>.
On Mon, 6 May 2002, Cliff Woolley wrote:
> > + ctx->boundary = apr_psprintf(r->pool, "%qx%lx",
> > + r->request_time, (long) getpid());
> Is %qx portable? Shouldn't that be APR_TIME_T_FMT?
Oh duh, this is apr_psprintf(), not printf(). Nevermind. ;)
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
Posted by Cliff Woolley <jw...@virginia.edu>.
On 6 May 2002 jerenkrantz@apache.org wrote:
> - Remove r->boundary since it is possible to have this self-contained in
> boundary's ctx. (May require MMN bump?)
That would definitely require an MMN bump.
> + ctx->boundary = apr_psprintf(r->pool, "%qx%lx",
> + r->request_time, (long) getpid());
Is %qx portable? Shouldn't that be APR_TIME_T_FMT?
> - if (ctx->bb) {
> + if (!APR_BRIGADE_EMPTY(ctx->bb)) {
> APR_BRIGADE_CONCAT(ctx->bb, bb);
> bb = ctx->bb;
> - ctx->bb = NULL; /* ### strictly necessary? call brigade_destroy? */
> }
> + apr_brigade_destroy(ctx->bb);
That apr_brigade_destroy() call is unnecessary. ctx->bb is allocated from
a pool, and you know it's empty because you just emptied it. There's
nothing to destroy. Ie, the answer to the ### comment was "no, and no."
:)
PS: If you're looking for a good way to test this stuff, run a big PDF
through it and view with Acrobat Reader. If Acrobat locks up, the
byterange filter is broken. ;)
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA