Hello!

On Fri, Nov 10, 2017 at 07:03:01PM +0800, 胡聪 (hucc) wrote:

> Hi,
>
> How about this as the first patch?
>
> # HG changeset patch
> # User hucongcong <hucong.c@foxmail.com>
> # Date 1510309868 -28800
> # Fri Nov 10 18:31:08 2017 +0800
> # Node ID c32fddd15a26b00f8f293f6b0d8762cd9f2bfbdb
> # Parent 32f83fe5747b55ef341595b18069bee3891874d0
> Range filter: support for multipart response in wider range.
>
> Before the patch multipart ranges are supported only if whole body
> is in a single buffer. Now, the limit is canceled. If there are no
> overlapping ranges and all ranges list in ascending order, nginx
> will return 206 with multipart response, otherwise return 200 (OK).

Introducing support for multipart ranges if the response body is
not in the single buffer as long as requested ranges do not
overlap and properly ordered looks like a much better idea to me.
That's basically what I have in mind as possible futher
enhancement of the range filter if we'll ever need better support
for multipart ranges.

There are various questions about the patch itself though, see
below.

> diff -r 32f83fe5747b -r c32fddd15a26 src/http/modules/ngx_http_range_filter_module.c
> --- a/src/http/modules/ngx_http_range_filter_module.c Fri Oct 27 00:30:38 2017 +0800
> +++ b/src/http/modules/ngx_http_range_filter_module.c Fri Nov 10 18:31:08 2017 +0800
> @@ -54,6 +54,7 @@ typedef struct {
>
> typedef struct {
> off_t offset;
> + ngx_uint_t index; /* start with 1 */
> ngx_str_t boundary_header;
> ngx_array_t ranges;
> } ngx_http_range_filter_ctx_t;
> @@ -66,12 +67,14 @@ static ngx_int_t ngx_http_range_singlepa
> static ngx_int_t ngx_http_range_multipart_header(ngx_http_request_t *r,
> ngx_http_range_filter_ctx_t *ctx);
> static ngx_int_t ngx_http_range_not_satisfiable(ngx_http_request_t *r);
> -static ngx_int_t ngx_http_range_test_overlapped(ngx_http_request_t *r,
> - ngx_http_range_filter_ctx_t *ctx, ngx_chain_t *in);
> static ngx_int_t ngx_http_range_singlepart_body(ngx_http_request_t *r,
> ngx_http_range_filter_ctx_t *ctx, ngx_chain_t *in);
> static ngx_int_t ngx_http_range_multipart_body(ngx_http_request_t *r,
> ngx_http_range_filter_ctx_t *ctx, ngx_chain_t *in);
> +static ngx_int_t ngx_http_range_link_boundary_header(ngx_http_request_t *r,
> + ngx_http_range_filter_ctx_t *ctx, ngx_chain_t ***lll);
> +static ngx_int_t ngx_http_range_link_last_boundary(ngx_http_request_t *r,
> + ngx_http_range_filter_ctx_t *ctx, ngx_chain_t **ll);
>
> static ngx_int_t ngx_http_range_header_filter_init(ngx_conf_t *cf);
> static ngx_int_t ngx_http_range_body_filter_init(ngx_conf_t *cf);
> @@ -270,9 +273,8 @@ ngx_http_range_parse(ngx_http_request_t
> ngx_uint_t ranges)
> {
> u_char *p;
> - off_t start, end, size, content_length, cutoff,
> - cutlim;
> - ngx_uint_t suffix;
> + off_t start, end, content_length, cutoff, cutlim;
> + ngx_uint_t suffix, descending;
> ngx_http_range_t *range;
> ngx_http_range_filter_ctx_t *mctx;
>
> @@ -281,6 +283,7 @@ ngx_http_range_parse(ngx_http_request_t
> ngx_http_range_body_filter_module);
> if (mctx) {
> ctx->ranges = mctx->ranges;
> + ctx->boundary_header = ctx->boundary_header;
> return NGX_OK;
> }
> }
> @@ -292,7 +295,8 @@ ngx_http_range_parse(ngx_http_request_t
> }
>
> p = r->headers_in.range->value.data + 6;
> - size = 0;
> + range = NULL;
> + descending = 0;
> content_length = r->headers_out.content_length_n;
>
> cutoff = NGX_MAX_OFF_T_VALUE / 10;
> @@ -369,6 +373,11 @@ ngx_http_range_parse(ngx_http_request_t
> found:
>
> if (start < end) {
> +
> + if (range && start < range->end) {
> + descending++;
> + }
> +
> range = ngx_array_push(&ctx->ranges);
> if (range == NULL) {
> return NGX_ERROR;
> @@ -377,16 +386,6 @@ ngx_http_range_parse(ngx_http_request_t
> range->start = start;
> range->end = end;
>
> - if (size > NGX_MAX_OFF_T_VALUE - (end - start)) {
> - return NGX_HTTP_RANGE_NOT_SATISFIABLE;
> - }
> -
> - size += end - start;
> -
> - if (ranges-- == 0) {
> - return NGX_DECLINED;
> - }
> -
> } else if (start == 0) {
> return NGX_DECLINED;
> }
> @@ -400,7 +399,7 @@ ngx_http_range_parse(ngx_http_request_t
> return NGX_HTTP_RANGE_NOT_SATISFIABLE;
> }
>
> - if (size > content_length) {
> + if (ctx->ranges.nelts > ranges || descending) {
> return NGX_DECLINED;
> }

This change basically disables support for non-ascending ranges.
As previously suggested, this will break various legitimate use
cases, and certainly this is not something we should do.

>
> @@ -469,6 +468,10 @@ ngx_http_range_multipart_header(ngx_http
> ngx_http_range_t *range;
> ngx_atomic_uint_t boundary;
>
> + if (r != r->main) {
> + return ngx_http_next_header_filter(r);
> + }
> +
> size = sizeof(CRLF "--") - 1 + NGX_ATOMIC_T_LEN
> + sizeof(CRLF "Content-Type: ") - 1
> + r->headers_out.content_type.len
> @@ -570,10 +573,11 @@ ngx_http_range_multipart_header(ngx_http
> - range[i].content_range.data;
>
> len += ctx->boundary_header.len + range[i].content_range.len
> - + (range[i].end - range[i].start);
> + + (range[i].end - range[i].start);

This looks like an unrelated whitespace change.

> }
>
> r->headers_out.content_length_n = len;
> + r->headers_out.content_offset = range[0].start;
>
> if (r->headers_out.content_length) {
> r->headers_out.content_length->hash = 0;
> @@ -639,63 +643,15 @@ ngx_http_range_body_filter(ngx_http_requ
> return ngx_http_range_singlepart_body(r, ctx, in);
> }
>
> - /*
> - * multipart ranges are supported only if whole body is in a single buffer
> - */
> -
> if (ngx_buf_special(in->buf)) {
> return ngx_http_next_body_filter(r, in);
> }

The ngx_buf_special() check should not be needed here as long as
ngx_http_range_multipart_body() is modified to properly support
multiple buffers.

>
> - if (ngx_http_range_test_overlapped(r, ctx, in) != NGX_OK) {
> - return NGX_ERROR;
> - }
> -
> return ngx_http_range_multipart_body(r, ctx, in);
> }
>
>
> static ngx_int_t
> -ngx_http_range_test_overlapped(ngx_http_request_t *r,
> - ngx_http_range_filter_ctx_t *ctx, ngx_chain_t *in)
> -{
> - off_t start, last;
> - ngx_buf_t *buf;
> - ngx_uint_t i;
> - ngx_http_range_t *range;
> -
> - if (ctx->offset) {
> - goto overlapped;
> - }
> -
> - buf = in->buf;
> -
> - if (!buf->last_buf) {
> - start = ctx->offset;
> - last = ctx->offset + ngx_buf_size(buf);
> -
> - range = ctx->ranges.elts;
> - for (i = 0; i < ctx->ranges.nelts; i++) {
> - if (start > range[i].start || last < range[i].end) {
> - goto overlapped;
> - }
> - }
> - }
> -
> - ctx->offset = ngx_buf_size(buf);
> -
> - return NGX_OK;
> -
> -overlapped:
> -
> - ngx_log_error(NGX_LOG_ALERT, r->connection->log, 0,
> - "range in overlapped buffers");
> -
> - return NGX_ERROR;
> -}
> -
> -
> -static ngx_int_t
> ngx_http_range_singlepart_body(ngx_http_request_t *r,
> ngx_http_range_filter_ctx_t *ctx, ngx_chain_t *in)
> {
> @@ -786,96 +742,227 @@ static ngx_int_t
> ngx_http_range_multipart_body(ngx_http_request_t *r,
> ngx_http_range_filter_ctx_t *ctx, ngx_chain_t *in)
> {
> - ngx_buf_t *b, *buf;
> - ngx_uint_t i;
> - ngx_chain_t *out, *hcl, *rcl, *dcl, **ll;
> - ngx_http_range_t *range;
> + off_t start, last, back;
> + ngx_buf_t *buf, *b;
> + ngx_uint_t i, finished;
> + ngx_chain_t *out, *cl, *ncl, **ll;
> + ngx_http_range_t *range, *tail;
>
> - ll = &out;
> - buf = in->buf;
> range = ctx->ranges.elts;
>
> - for (i = 0; i < ctx->ranges.nelts; i++) {
> + if (!ctx->index) {
> + for (i = 0; i < ctx->ranges.nelts; i++) {
> + if (ctx->offset < range[i].end) {
> + ctx->index = i + 1;
> + break;
> + }
> + }
> + }

All this logic with using ctx->index as range index plus 1 looks
counter-intuitive and unneeded. A much better options would be
(in no particular order):

- use a special value to mean "uninitialized", like -1;

- always initialize ctx->index to 0 and move it futher to the next
range once we see that ctx->offset is larger than range[i].end;

- do proper initialization somewhere in
ngx_http_range_header_filter() or ngx_http_range_multipart_header().

> +
> + tail = range + ctx->ranges.nelts - 1;
> + range += ctx->index - 1;
> +
> + out = NULL;
> + ll = &out;
> + finished = 0;
>
> - /*
> - * The boundary header of the range:
> - * CRLF
> - * "--0123456789" CRLF
> - * "Content-Type: image/jpeg" CRLF
> - * "Content-Range: bytes "
> - */
> + for (cl = in; cl; cl = cl->next) {
> +
> + buf = cl->buf;
> +
> + start = ctx->offset;
> + last = ctx->offset + ngx_buf_size(buf);
>
> - b = ngx_calloc_buf(r->pool);
> - if (b == NULL) {
> - return NGX_ERROR;
> + ctx->offset = last;
> +
> + ngx_log_debug2(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
> + "http range multipart body buf: %O-%O", start, last);
> +
> + if (ngx_buf_special(buf)) {
> + *ll = cl;
> + ll = &cl->next;
> + continue;
> }
>
> - b->memory = 1;
> - b->pos = ctx->boundary_header.data;
> - b->last = ctx->boundary_header.data + ctx->boundary_header.len;
> + if (range->end <= start || range->start >= last) {
> +
> + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
> + "http range multipart body skip");
>
> - hcl = ngx_alloc_chain_link(r->pool);
> - if (hcl == NULL) {
> - return NGX_ERROR;
> + if (buf->in_file) {
> + buf->file_pos = buf->file_last;
> + }
> +
> + buf->pos = buf->last;
> + buf->sync = 1;
> +
> + continue;

Looking at this code I tend to think that our existing
ngx_http_range_singlepart_body() implementation you've used as a
reference is incorrect. It removes buffers from the original
chain as passed to the filter - this can result in a buffer being
lost from tracking by the module who owns the buffer, and a
request hang if/when all available buffers will be lost. Instead,
it should either preserve all existing chain links, or create a
new chain. I'll take a look how to properly fix this.

> }
>
> - hcl->buf = b;
> + if (range->start >= start) {
>
> + if (ngx_http_range_link_boundary_header(r, ctx, &ll) != NGX_OK) {
> + return NGX_ERROR;
> + }
>
> - /* "SSSS-EEEE/TTTT" CRLF CRLF */
> + if (buf->in_file) {
> + buf->file_pos += range->start - start;
> + }
>
> - b = ngx_calloc_buf(r->pool);
> - if (b == NULL) {
> - return NGX_ERROR;
> + if (ngx_buf_in_memory(buf)) {
> + buf->pos += (size_t) (range->start - start);
> + }
> }
>
> - b->temporary = 1;
> - b->pos = range[i].content_range.data;
> - b->last = range[i].content_range.data + range[i].content_range.len;
> + if (range->end <= last) {
> +
> + if (range < tail && range[1].start < last) {

The "tail" name is not immediately obvious, and it might be better
idea to name it differently. Also, range[1] looks strange when we
are using range as a pointer and not array. Hopefully this test
will be unneeded when code will be cleaned up to avoid moving
ctx->offset backwards, see below.

> +
> + b = ngx_alloc_buf(r->pool);
> + if (b == NULL) {
> + return NGX_ERROR;
> + }
> +
> + ncl = ngx_alloc_chain_link(r->pool);
> + if (ncl == NULL) {
> + return NGX_ERROR;
> + }

Note: usual names for temporary chain links are "ln" and "tl".

>
> - rcl = ngx_alloc_chain_link(r->pool);
> - if (rcl == NULL) {
> - return NGX_ERROR;
> - }
> + ncl->buf = b;
> + ncl->next = cl;
> +
> + ngx_memcpy(b, buf, sizeof(ngx_buf_t));
> + b->last_in_chain = 0;
> + b->last_buf = 0;
> +
> + back = last - range->end;
> + ctx->offset -= back;

This looks like a hack, there should be no need to adjust
ctx->offset backwards. Instead, we should move ctx->offset only
when we've done with a buffer.

> +
> + ngx_log_debug2(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
> + "http range multipart body reuse buf: %O-%O",
> + ctx->offset, ctx->offset + back);
>
> - rcl->buf = b;
> + if (buf->in_file) {
> + buf->file_pos = buf->file_last - back;
> + }
> +
> + if (ngx_buf_in_memory(buf)) {
> + buf->pos = buf->last - back;
> + }
>
> + cl = ncl;
> + buf = cl->buf;
> + }
> +
> + if (buf->in_file) {
> + buf->file_last -= last - range->end;
> + }
>
> - /* the range data */
> + if (ngx_buf_in_memory(buf)) {
> + buf->last -= (size_t) (last - range->end);
> + }
>
> - b = ngx_calloc_buf(r->pool);
> - if (b == NULL) {
> - return NGX_ERROR;
> + if (range == tail) {
> + buf->last_buf = (r == r->main) ? 1 : 0;
> + buf->last_in_chain = 1;
> + *ll = cl;
> + ll = &cl->next;
> +
> + finished = 1;

It is not clear why to use the "finished" flag instead of adding
the boundary here.

> + break;
> + }
> +
> + range++;
> + ctx->index++;
> }
>
> - b->in_file = buf->in_file;
> - b->temporary = buf->temporary;
> - b->memory = buf->memory;
> - b->mmap = buf->mmap;
> - b->file = buf->file;
> + *ll = cl;
> + ll = &cl->next;
> + }
> +
> + if (out == NULL) {
> + return NGX_OK;
> + }
> +
> + *ll = NULL;
> +
> + if (finished
> + && ngx_http_range_link_last_boundary(r, ctx, ll) != NGX_OK)
> + {
> + return NGX_ERROR;
> + }
> +
> + return ngx_http_next_body_filter(r, out);
> +}
> +
>
> - if (buf->in_file) {
> - b->file_pos = buf->file_pos + range[i].start;
> - b->file_last = buf->file_pos + range[i].end;
> - }
> +static ngx_int_t
> +ngx_http_range_link_boundary_header(ngx_http_request_t *r,
> + ngx_http_range_filter_ctx_t *ctx, ngx_chain_t ***lll)

The "ngx_chain_t ***lll" argument suggests it might be a good idea
to somehow improve the interface.

> +{
> + ngx_buf_t *b;
> + ngx_chain_t *hcl, *rcl;
> + ngx_http_range_t *range;
> +
> + /*
> + * The boundary header of the range:
> + * CRLF
> + * "--0123456789" CRLF
> + * "Content-Type: image/jpeg" CRLF
> + * "Content-Range: bytes "
> + */
> +
> + b = ngx_calloc_buf(r->pool);
> + if (b == NULL) {
> + return NGX_ERROR;
> + }
>
> - if (ngx_buf_in_memory(buf)) {
> - b->pos = buf->pos + (size_t) range[i].start;
> - b->last = buf->pos + (size_t) range[i].end;
> - }
> + b->memory = 1;
> + b->pos = ctx->boundary_header.data;
> + b->last = ctx->boundary_header.data + ctx->boundary_header.len;
> +
> + hcl = ngx_alloc_chain_link(r->pool);
> + if (hcl == NULL) {
> + return NGX_ERROR;
> + }
> +
> + hcl->buf = b;
> +
> +
> + /* "SSSS-EEEE/TTTT" CRLF CRLF */
> +
> + b = ngx_calloc_buf(r->pool);
> + if (b == NULL) {
> + return NGX_ERROR;
> + }
>
> - dcl = ngx_alloc_chain_link(r->pool);
> - if (dcl == NULL) {
> - return NGX_ERROR;
> - }
> + range = ctx->ranges.elts;
> + b->temporary = 1;
> + b->pos = range[ctx->index - 1].content_range.data;
> + b->last = range[ctx->index - 1].content_range.data
> + + range[ctx->index - 1].content_range.len;
> +
> + rcl = ngx_alloc_chain_link(r->pool);
> + if (rcl == NULL) {
> + return NGX_ERROR;
> + }
> +
> + rcl->buf = b;
>
> - dcl->buf = b;
> + **lll = hcl;
> + hcl->next = rcl;
> + *lll = &rcl->next;
> +
> + return NGX_OK;
> +}
>
> - *ll = hcl;
> - hcl->next = rcl;
> - rcl->next = dcl;
> - ll = &dcl->next;
> - }
> +
> +static ngx_int_t
> +ngx_http_range_link_last_boundary(ngx_http_request_t *r,
> + ngx_http_range_filter_ctx_t *ctx, ngx_chain_t **ll)
> +{
> + ngx_buf_t *b;
> + ngx_chain_t *hcl;
>
> /* the last boundary CRLF "--0123456789--" CRLF */
>
> @@ -885,7 +972,8 @@ ngx_http_range_multipart_body(ngx_http_r
> }
>
> b->temporary = 1;
> - b->last_buf = 1;
> + b->last_in_chain = 1;
> + b->last_buf = (r == r->main) ? 1 : 0;
>
> b->pos = ngx_pnalloc(r->pool, sizeof(CRLF "--") - 1 + NGX_ATOMIC_T_LEN
> + sizeof("--" CRLF) - 1);
> @@ -908,7 +996,7 @@ ngx_http_range_multipart_body(ngx_http_r
>
> *ll = hcl;
>
> - return ngx_http_next_body_filter(r, out);
> + return NGX_OK;
> }
>
>
> ------------------ Original ------------------
> From: "胡聪 (hucc)";<hucong.c@foxmail.com>;
> Send time: Friday, Nov 10, 2017 4:41 AM
> To: "nginx-devel"<nginx-devel@nginx.org>;
> Subject: Re: [patch-1] Range filter: support multiple ranges.
>
> Hi,
>
> Please ignore the previous reply. The updated patch is placed at the end.
>
> On Thursday, Nov 9, 2017 10:48 PM +0300 Maxim Dounin wrote:
>
> >On Fri, Oct 27, 2017 at 06:50:32PM +0800, 胡聪 (hucc) wrote:
> >
> >> # HG changeset patch
> >> # User hucongcong <hucong.c@foxmail.com>
> >> # Date 1509099940 -28800
> >> # Fri Oct 27 18:25:40 2017 +0800
> >> # Node ID 62c100a0d42614cd46f0719c0acb0ad914594217
> >> # Parent b9850d3deb277bd433a689712c40a84401443520
> >> Range filter: support multiple ranges.
> >
> >This summary line is at least misleading.
>
> Ok, maybe the summary line is support multiple ranges when body is
> in multiple buffers.
>
> >> When multiple ranges are requested, nginx will coalesce any of the ranges
> >> that overlap, or that are separated by a gap that is smaller than the
> >> NGX_HTTP_RANGE_MULTIPART_GAP macro.
> >
> >(Note that the patch also does reordering of ranges. For some
> >reason this is not mentioned in the commit log. There are also
> >other changes not mentioned in the commit log - for example, I see
> >ngx_http_range_t was moved to ngx_http_request.h. These are
> >probably do not belong to the patch at all.)
>
> I actually wait for you to give better advice. I tried my best to
> make the changes easier and more readable and I will split it into
> multiple patches based on your suggestions if these changes will be
> accepted.
>
> >Reordering and/or coalescing ranges is not something that clients
> >usually expect to happen. This was widely discussed at the time
> >of CVE-2011-3192 vulnerability in Apache. As a result, RFC 7233
> >introduced the "MAY coalesce" clause. But this doesn't make
> >clients, especially old ones, magically prepared for this.
>
> I did not know the CVE-2011-3192. If multiple ranges list in
> ascending order and there are no overlapping ranges, the code will
> be much simpler. This is what I think.
>
> >Moreover, this will certainly break some use cases like "request
> >some metadata first, and then rest of the file". So this is
> >certainly not a good idea to always reorder / coalesce ranges
> >unless this is really needed for some reason. (Or even at all,
> >as just returning 200 might be much more compatible with various
> >clients, as outlined above.)
> >
> >It is also not clear what you are trying to achieve with this
> >patch. You may want to elaborate more on what problem you are
> >trying to solve, may be there are better solutions.
>
> I am trying to support multiple ranges when proxy_buffering is off
> and sometimes slice is enabled. The data is always cached in the
> backend which is not nginx. As far as I know, similar architecture
> is widely used in CDN. So the implementation of multiple ranges in
> the architecture I mentioned above is required and inevitable.
> Besides, P2P clients desire for this feature to gather data-pieces.
> Hope I already made it clear.
>
> All these changes have been tested. Hope it helps! Temporarily,
> the changes are as follows:
>
> diff -r 32f83fe5747b src/http/modules/ngx_http_range_filter_module.c
> --- a/src/http/modules/ngx_http_range_filter_module.c Fri Oct 27 00:30:38 2017 +0800
> +++ b/src/http/modules/ngx_http_range_filter_module.c Fri Nov 10 04:31:52 2017 +0800
> @@ -46,16 +46,10 @@
>
>
> typedef struct {
> - off_t start;
> - off_t end;
> - ngx_str_t content_range;
> -} ngx_http_range_t;
> + off_t offset;
> + ngx_uint_t index; /* start with 1 */
>
> -
> -typedef struct {
> - off_t offset;
> - ngx_str_t boundary_header;
> - ngx_array_t ranges;
> + ngx_str_t boundary_header;
> } ngx_http_range_filter_ctx_t;
>
>
> @@ -66,12 +60,14 @@ static ngx_int_t ngx_http_range_singlepa
> static ngx_int_t ngx_http_range_multipart_header(ngx_http_request_t *r,
> ngx_http_range_filter_ctx_t *ctx);
> static ngx_int_t ngx_http_range_not_satisfiable(ngx_http_request_t *r);
> -static ngx_int_t ngx_http_range_test_overlapped(ngx_http_request_t *r,
> - ngx_http_range_filter_ctx_t *ctx, ngx_chain_t *in);
> static ngx_int_t ngx_http_range_singlepart_body(ngx_http_request_t *r,
> ngx_http_range_filter_ctx_t *ctx, ngx_chain_t *in);
> static ngx_int_t ngx_http_range_multipart_body(ngx_http_request_t *r,
> ngx_http_range_filter_ctx_t *ctx, ngx_chain_t *in);
> +static ngx_int_t ngx_http_range_link_boundary_header(ngx_http_request_t *r,
> + ngx_http_range_filter_ctx_t *ctx, ngx_chain_t ***lll);
> +static ngx_int_t ngx_http_range_link_last_boundary(ngx_http_request_t *r,
> + ngx_http_range_filter_ctx_t *ctx, ngx_chain_t **ll);
>
> static ngx_int_t ngx_http_range_header_filter_init(ngx_conf_t *cf);
> static ngx_int_t ngx_http_range_body_filter_init(ngx_conf_t *cf);
> @@ -234,7 +230,7 @@ parse:
> r->headers_out.status = NGX_HTTP_PARTIAL_CONTENT;
> r->headers_out.status_line.len = 0;
>
> - if (ctx->ranges.nelts == 1) {
> + if (r->headers_out.ranges->nelts == 1) {
> return ngx_http_range_singlepart_header(r, ctx);
> }
>
> @@ -270,9 +266,9 @@ ngx_http_range_parse(ngx_http_request_t
> ngx_uint_t ranges)
> {
> u_char *p;
> - off_t start, end, size, content_length, cutoff,
> - cutlim;
> - ngx_uint_t suffix;
> + off_t start, end, content_length,
> + cutoff, cutlim;
> + ngx_uint_t suffix, descending;
> ngx_http_range_t *range;
> ngx_http_range_filter_ctx_t *mctx;
>
> @@ -280,19 +276,21 @@ ngx_http_range_parse(ngx_http_request_t
> mctx = ngx_http_get_module_ctx(r->main,
> ngx_http_range_body_filter_module);
> if (mctx) {
> - ctx->ranges = mctx->ranges;
> + r->headers_out.ranges = r->main->headers_out.ranges;
> + ctx->boundary_header = mctx->boundary_header;
> return NGX_OK;
> }
> }
>
> - if (ngx_array_init(&ctx->ranges, r->pool, 1, sizeof(ngx_http_range_t))
> - != NGX_OK)
> - {
> + r->headers_out.ranges = ngx_array_create(r->pool, 1,
> + sizeof(ngx_http_range_t));
> + if (r->headers_out.ranges == NULL) {
> return NGX_ERROR;
> }
>
> p = r->headers_in.range->value.data + 6;
> - size = 0;
> + range = NULL;
> + descending = 0;
> content_length = r->headers_out.content_length_n;
>
> cutoff = NGX_MAX_OFF_T_VALUE / 10;
> @@ -369,7 +367,12 @@ ngx_http_range_parse(ngx_http_request_t
> found:
>
> if (start < end) {
> - range = ngx_array_push(&ctx->ranges);
> +
> + if (range && start < range->end) {
> + descending++;
> + }
> +
> + range = ngx_array_push(r->headers_out.ranges);
> if (range == NULL) {
> return NGX_ERROR;
> }
> @@ -377,16 +380,6 @@ ngx_http_range_parse(ngx_http_request_t
> range->start = start;
> range->end = end;
>
> - if (size > NGX_MAX_OFF_T_VALUE - (end - start)) {
> - return NGX_HTTP_RANGE_NOT_SATISFIABLE;
> - }
> -
> - size += end - start;
> -
> - if (ranges-- == 0) {
> - return NGX_DECLINED;
> - }
> -
> } else if (start == 0) {
> return NGX_DECLINED;
> }
> @@ -396,11 +389,15 @@ ngx_http_range_parse(ngx_http_request_t
> }
> }
>
> - if (ctx->ranges.nelts == 0) {
> + if (r->headers_out.ranges->nelts == 0) {
> return NGX_HTTP_RANGE_NOT_SATISFIABLE;
> }
>
> - if (size > content_length) {
> + if (r->headers_out.ranges->nelts > ranges) {
> + r->headers_out.ranges->nelts = ranges;
> + }
> +
> + if (descending) {
> return NGX_DECLINED;
> }
>
> @@ -439,7 +436,7 @@ ngx_http_range_singlepart_header(ngx_htt
>
> /* "Content-Range: bytes SSSS-EEEE/TTTT" header */
>
> - range = ctx->ranges.elts;
> + range = r->headers_out.ranges->elts;
>
> content_range->value.len = ngx_sprintf(content_range->value.data,
> "bytes %O-%O/%O",
> @@ -469,6 +466,10 @@ ngx_http_range_multipart_header(ngx_http
> ngx_http_range_t *range;
> ngx_atomic_uint_t boundary;
>
> + if (r != r->main) {
> + return ngx_http_next_header_filter(r);
> + }
> +
> size = sizeof(CRLF "--") - 1 + NGX_ATOMIC_T_LEN
> + sizeof(CRLF "Content-Type: ") - 1
> + r->headers_out.content_type.len
> @@ -551,8 +552,8 @@ ngx_http_range_multipart_header(ngx_http
>
> len = sizeof(CRLF "--") - 1 + NGX_ATOMIC_T_LEN + sizeof("--" CRLF) - 1;
>
> - range = ctx->ranges.elts;
> - for (i = 0; i < ctx->ranges.nelts; i++) {
> + range = r->headers_out.ranges->elts;
> + for (i = 0; i < r->headers_out.ranges->nelts; i++) {
>
> /* the size of the range: "SSSS-EEEE/TTTT" CRLF CRLF */
>
> @@ -570,10 +571,11 @@ ngx_http_range_multipart_header(ngx_http
> - range[i].content_range.data;
>
> len += ctx->boundary_header.len + range[i].content_range.len
> - + (range[i].end - range[i].start);
> + + (range[i].end - range[i].start);
> }
>
> r->headers_out.content_length_n = len;
> + r->headers_out.content_offset = range[0].start;
>
> if (r->headers_out.content_length) {
> r->headers_out.content_length->hash = 0;
> @@ -635,67 +637,19 @@ ngx_http_range_body_filter(ngx_http_requ
> return ngx_http_next_body_filter(r, in);
> }
>
> - if (ctx->ranges.nelts == 1) {
> + if (r->headers_out.ranges->nelts == 1) {
> return ngx_http_range_singlepart_body(r, ctx, in);
> }
>
> - /*
> - * multipart ranges are supported only if whole body is in a single buffer
> - */
> -
> if (ngx_buf_special(in->buf)) {
> return ngx_http_next_body_filter(r, in);
> }
>
> - if (ngx_http_range_test_overlapped(r, ctx, in) != NGX_OK) {
> - return NGX_ERROR;
> - }
> -
> return ngx_http_range_multipart_body(r, ctx, in);
> }
>
>
> static ngx_int_t
> -ngx_http_range_test_overlapped(ngx_http_request_t *r,
> - ngx_http_range_filter_ctx_t *ctx, ngx_chain_t *in)
> -{
> - off_t start, last;
> - ngx_buf_t *buf;
> - ngx_uint_t i;
> - ngx_http_range_t *range;
> -
> - if (ctx->offset) {
> - goto overlapped;
> - }
> -
> - buf = in->buf;
> -
> - if (!buf->last_buf) {
> - start = ctx->offset;
> - last = ctx->offset + ngx_buf_size(buf);
> -
> - range = ctx->ranges.elts;
> - for (i = 0; i < ctx->ranges.nelts; i++) {
> - if (start > range[i].start || last < range[i].end) {
> - goto overlapped;
> - }
> - }
> - }
> -
> - ctx->offset = ngx_buf_size(buf);
> -
> - return NGX_OK;
> -
> -overlapped:
> -
> - ngx_log_error(NGX_LOG_ALERT, r->connection->log, 0,
> - "range in overlapped buffers");
> -
> - return NGX_ERROR;
> -}
> -
> -
> -static ngx_int_t
> ngx_http_range_singlepart_body(ngx_http_request_t *r,
> ngx_http_range_filter_ctx_t *ctx, ngx_chain_t *in)
> {
> @@ -706,7 +660,7 @@ ngx_http_range_singlepart_body(ngx_http_
>
> out = NULL;
> ll = &out;
> - range = ctx->ranges.elts;
> + range = r->headers_out.ranges->elts;
>
> for (cl = in; cl; cl = cl->next) {
>
> @@ -786,96 +740,227 @@ static ngx_int_t
> ngx_http_range_multipart_body(ngx_http_request_t *r,
> ngx_http_range_filter_ctx_t *ctx, ngx_chain_t *in)
> {
> - ngx_buf_t *b, *buf;
> - ngx_uint_t i;
> - ngx_chain_t *out, *hcl, *rcl, *dcl, **ll;
> - ngx_http_range_t *range;
> + off_t start, last, back;
> + ngx_buf_t *buf, *b;
> + ngx_uint_t i, finished;
> + ngx_chain_t *out, *cl, *ncl, **ll;
> + ngx_http_range_t *range, *tail;
> +
> + range = r->headers_out.ranges->elts;
>
> - ll = &out;
> - buf = in->buf;
> - range = ctx->ranges.elts;
> + if (!ctx->index) {
> + for (i = 0; i < r->headers_out.ranges->nelts; i++) {
> + if (ctx->offset < range[i].end) {
> + ctx->index = i + 1;
> + break;
> + }
> + }
> + }
>
> - for (i = 0; i < ctx->ranges.nelts; i++) {
> + tail = range + r->headers_out.ranges->nelts - 1;
> + range += ctx->index - 1;
>
> - /*
> - * The boundary header of the range:
> - * CRLF
> - * "--0123456789" CRLF
> - * "Content-Type: image/jpeg" CRLF
> - * "Content-Range: bytes "
> - */
> + out = NULL;
> + ll = &out;
> + finished = 0;
> +
> + for (cl = in; cl; cl = cl->next) {
> +
> + buf = cl->buf;
>
> - b = ngx_calloc_buf(r->pool);
> - if (b == NULL) {
> - return NGX_ERROR;
> + start = ctx->offset;
> + last = ctx->offset + ngx_buf_size(buf);
> +
> + ctx->offset = last;
> +
> + ngx_log_debug2(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
> + "http range multipart body buf: %O-%O", start, last);
> +
> + if (ngx_buf_special(buf)) {
> + *ll = cl;
> + ll = &cl->next;
> + continue;
> }
>
> - b->memory = 1;
> - b->pos = ctx->boundary_header.data;
> - b->last = ctx->boundary_header.data + ctx->boundary_header.len;
> + if (range->end <= start || range->start >= last) {
> +
> + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
> + "http range multipart body skip");
>
> - hcl = ngx_alloc_chain_link(r->pool);
> - if (hcl == NULL) {
> - return NGX_ERROR;
> + if (buf->in_file) {
> + buf->file_pos = buf->file_last;
> + }
> +
> + buf->pos = buf->last;
> + buf->sync = 1;
> +
> + continue;
> }
>
> - hcl->buf = b;
> + if (range->start >= start) {
>
> + if (ngx_http_range_link_boundary_header(r, ctx, &ll) != NGX_OK) {
> + return NGX_ERROR;
> + }
>
> - /* "SSSS-EEEE/TTTT" CRLF CRLF */
> + if (buf->in_file) {
> + buf->file_pos += range->start - start;
> + }
>
> - b = ngx_calloc_buf(r->pool);
> - if (b == NULL) {
> - return NGX_ERROR;
> + if (ngx_buf_in_memory(buf)) {
> + buf->pos += (size_t) (range->start - start);
> + }
> }
>
> - b->temporary = 1;
> - b->pos = range[i].content_range.data;
> - b->last = range[i].content_range.data + range[i].content_range.len;
> + if (range->end <= last) {
> +
> + if (range < tail && range[1].start < last) {
> +
> + b = ngx_alloc_buf(r->pool);
> + if (b == NULL) {
> + return NGX_ERROR;
> + }
> +
> + ncl = ngx_alloc_chain_link(r->pool);
> + if (ncl == NULL) {
> + return NGX_ERROR;
> + }
>
> - rcl = ngx_alloc_chain_link(r->pool);
> - if (rcl == NULL) {
> - return NGX_ERROR;
> - }
> + ncl->buf = b;
> + ncl->next = cl;
> +
> + ngx_memcpy(b, buf, sizeof(ngx_buf_t));
> + b->last_in_chain = 0;
> + b->last_buf = 0;
> +
> + back = last - range->end;
> + ctx->offset -= back;
> +
> + ngx_log_debug2(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
> + "http range multipart body reuse buf: %O-%O",
> + ctx->offset, ctx->offset + back);
>
> - rcl->buf = b;
> + if (buf->in_file) {
> + buf->file_pos = buf->file_last - back;
> + }
> +
> + if (ngx_buf_in_memory(buf)) {
> + buf->pos = buf->last - back;
> + }
>
> + cl = ncl;
> + buf = cl->buf;
> + }
> +
> + if (buf->in_file) {
> + buf->file_last -= last - range->end;
> + }
>
> - /* the range data */
> + if (ngx_buf_in_memory(buf)) {
> + buf->last -= (size_t) (last - range->end);
> + }
>
> - b = ngx_calloc_buf(r->pool);
> - if (b == NULL) {
> - return NGX_ERROR;
> + if (range == tail) {
> + buf->last_buf = (r == r->main) ? 1 : 0;
> + buf->last_in_chain = 1;
> + *ll = cl;
> + ll = &cl->next;
> +
> + finished = 1;
> + break;
> + }
> +
> + range++;
> + ctx->index++;
> }
>
> - b->in_file = buf->in_file;
> - b->temporary = buf->temporary;
> - b->memory = buf->memory;
> - b->mmap = buf->mmap;
> - b->file = buf->file;
> + *ll = cl;
> + ll = &cl->next;
> + }
> +
> + if (out == NULL) {
> + return NGX_OK;
> + }
> +
> + *ll = NULL;
> +
> + if (finished
> + && ngx_http_range_link_last_boundary(r, ctx, ll) != NGX_OK)
> + {
> + return NGX_ERROR;
> + }
> +
> + return ngx_http_next_body_filter(r, out);
> +}
> +
>
> - if (buf->in_file) {
> - b->file_pos = buf->file_pos + range[i].start;
> - b->file_last = buf->file_pos + range[i].end;
> - }
> +static ngx_int_t
> +ngx_http_range_link_boundary_header(ngx_http_request_t *r,
> + ngx_http_range_filter_ctx_t *ctx, ngx_chain_t ***lll)
> +{
> + ngx_buf_t *b;
> + ngx_chain_t *hcl, *rcl;
> + ngx_http_range_t *range;
> +
> + /*
> + * The boundary header of the range:
> + * CRLF
> + * "--0123456789" CRLF
> + * "Content-Type: image/jpeg" CRLF
> + * "Content-Range: bytes "
> + */
> +
> + b = ngx_calloc_buf(r->pool);
> + if (b == NULL) {
> + return NGX_ERROR;
> + }
>
> - if (ngx_buf_in_memory(buf)) {
> - b->pos = buf->pos + (size_t) range[i].start;
> - b->last = buf->pos + (size_t) range[i].end;
> - }
> + b->memory = 1;
> + b->pos = ctx->boundary_header.data;
> + b->last = ctx->boundary_header.data + ctx->boundary_header.len;
> +
> + hcl = ngx_alloc_chain_link(r->pool);
> + if (hcl == NULL) {
> + return NGX_ERROR;
> + }
> +
> + hcl->buf = b;
> +
> +
> + /* "SSSS-EEEE/TTTT" CRLF CRLF */
> +
> + b = ngx_calloc_buf(r->pool);
> + if (b == NULL) {
> + return NGX_ERROR;
> + }
>
> - dcl = ngx_alloc_chain_link(r->pool);
> - if (dcl == NULL) {
> - return NGX_ERROR;
> - }
> + range = r->headers_out.ranges->elts;
> + b->temporary = 1;
> + b->pos = range[ctx->index - 1].content_range.data;
> + b->last = range[ctx->index - 1].content_range.data
> + + range[ctx->index - 1].content_range.len;
> +
> + rcl = ngx_alloc_chain_link(r->pool);
> + if (rcl == NULL) {
> + return NGX_ERROR;
> + }
> +
> + rcl->buf = b;
>
> - dcl->buf = b;
> + **lll = hcl;
> + hcl->next = rcl;
> + *lll = &rcl->next;
> +
> + return NGX_OK;
> +}
>
> - *ll = hcl;
> - hcl->next = rcl;
> - rcl->next = dcl;
> - ll = &dcl->next;
> - }
> +
> +static ngx_int_t
> +ngx_http_range_link_last_boundary(ngx_http_request_t *r,
> + ngx_http_range_filter_ctx_t *ctx, ngx_chain_t **ll)
> +{
> + ngx_buf_t *b;
> + ngx_chain_t *hcl;
>
> /* the last boundary CRLF "--0123456789--" CRLF */
>
> @@ -885,7 +970,8 @@ ngx_http_range_multipart_body(ngx_http_r
> }
>
> b->temporary = 1;
> - b->last_buf = 1;
> + b->last_in_chain = 1;
> + b->last_buf = (r == r->main) ? 1 : 0;
>
> b->pos = ngx_pnalloc(r->pool, sizeof(CRLF "--") - 1 + NGX_ATOMIC_T_LEN
> + sizeof("--" CRLF) - 1);
> @@ -908,7 +994,7 @@ ngx_http_range_multipart_body(ngx_http_r
>
> *ll = hcl;
>
> - return ngx_http_next_body_filter(r, out);
> + return NGX_OK;
> }
>
>
> diff -r 32f83fe5747b src/http/modules/ngx_http_slice_filter_module.c
> --- a/src/http/modules/ngx_http_slice_filter_module.c Fri Oct 27 00:30:38 2017 +0800
> +++ b/src/http/modules/ngx_http_slice_filter_module.c Fri Nov 10 04:31:52 2017 +0800
> @@ -22,6 +22,8 @@ typedef struct {
> ngx_str_t etag;
> unsigned last:1;
> unsigned active:1;
> + unsigned multipart:1;
> + ngx_uint_t index;
> ngx_http_request_t *sr;
> } ngx_http_slice_ctx_t;
>
> @@ -103,7 +105,9 @@ ngx_http_slice_header_filter(ngx_http_re
> {
> off_t end;
> ngx_int_t rc;
> + ngx_uint_t i;
> ngx_table_elt_t *h;
> + ngx_http_range_t *range;
> ngx_http_slice_ctx_t *ctx;
> ngx_http_slice_loc_conf_t *slcf;
> ngx_http_slice_content_range_t cr;
> @@ -182,27 +186,48 @@ ngx_http_slice_header_filter(ngx_http_re
>
> r->allow_ranges = 1;
> r->subrequest_ranges = 1;
> - r->single_range = 1;
>
> rc = ngx_http_next_header_filter(r);
>
> - if (r != r->main) {
> - return rc;
> + if (r == r->main) {
> + r->preserve_body = 1;
> +
> + if (r->headers_out.status == NGX_HTTP_PARTIAL_CONTENT) {
> + ctx->multipart = (r->headers_out.ranges->nelts != 1);
> + range = r->headers_out.ranges->elts;
> +
> + if (ctx->start + (off_t) slcf->size <= range[0].start) {
> + ctx->start = slcf->size * (range[0].start / slcf->size);
> + }
> +
> + ctx->end = range[r->headers_out.ranges->nelts - 1].end;
> +
> + } else {
> + ctx->end = cr.complete_length;
> + }
> }
>
> - r->preserve_body = 1;
> + if (ctx->multipart) {
> + range = r->headers_out.ranges->elts;
> +
> + for (i = ctx->index; i < r->headers_out.ranges->nelts - 1; i++) {
> +
> + if (ctx->start < range[i].end) {
> + ctx->index = i;
> + break;
> + }
>
> - if (r->headers_out.status == NGX_HTTP_PARTIAL_CONTENT) {
> - if (ctx->start + (off_t) slcf->size <= r->headers_out.content_offset) {
> - ctx->start = slcf->size
> - * (r->headers_out.content_offset / slcf->size);
> + if (ctx->start + (off_t) slcf->size <= range[i + 1].start) {
> + i++;
> + ctx->index = i;
> + ctx->start = slcf->size * (range[i].start / slcf->size);
> +
> + ngx_log_debug3(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
> + "range multipart so fast forward to %O-%O @%O",
> + range[i].start, range[i].end, ctx->start);
> + break;
> + }
> }
> -
> - ctx->end = r->headers_out.content_offset
> - + r->headers_out.content_length_n;
> -
> - } else {
> - ctx->end = cr.complete_length;
> }
>
> return rc;
> diff -r 32f83fe5747b src/http/ngx_http_request.h
> --- a/src/http/ngx_http_request.h Fri Oct 27 00:30:38 2017 +0800
> +++ b/src/http/ngx_http_request.h Fri Nov 10 04:31:52 2017 +0800
> @@ -251,6 +251,13 @@ typedef struct {
>
>
> typedef struct {
> + off_t start;
> + off_t end;
> + ngx_str_t content_range;
> +} ngx_http_range_t;
> +
> +
> +typedef struct {
> ngx_list_t headers;
> ngx_list_t trailers;
>
> @@ -278,6 +285,7 @@ typedef struct {
> u_char *content_type_lowcase;
> ngx_uint_t content_type_hash;
>
> + ngx_array_t *ranges; /* ngx_http_range_t */
> ngx_array_t cache_control;
>
> off_t content_length_n;
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel