httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: svn commit: r1161661 - /httpd/httpd/trunk/modules/http/byterange_filter.c
Date Fri, 26 Aug 2011 06:50:06 GMT


On 08/25/2011 07:38 PM, jim@apache.org wrote:
> Author: jim
> Date: Thu Aug 25 17:38:19 2011
> New Revision: 1161661
> 
> URL: http://svn.apache.org/viewvc?rev=1161661&view=rev
> Log:
> Merge in byteranges
> 
> Modified:
>     httpd/httpd/trunk/modules/http/byterange_filter.c
> 
> Modified: httpd/httpd/trunk/modules/http/byterange_filter.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/byterange_filter.c?rev=1161661&r1=1161660&r2=1161661&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/http/byterange_filter.c (original)
> +++ httpd/httpd/trunk/modules/http/byterange_filter.c Thu Aug 25 17:38:19 2011
> @@ -469,11 +469,14 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
>  
>  static int ap_set_byterange(request_rec *r)
>  {
> -    const char *range;
> +    const char *range, *or;
>      const char *if_range;
>      const char *match;
>      const char *ct;
> -    int num_ranges;
> +    char *merged, *cur;
> +    int num_ranges = 0;
> +    apr_off_t ostart, oend;
> +    int in_merge = 0;
>  
>      if (r->assbackwards) {
>          return 0;
> @@ -526,17 +529,81 @@ static int ap_set_byterange(request_rec 
>          }
>      }
>  
> -    if (!ap_strchr_c(range, ',')) {
> -        /* a single range */
> -        num_ranges = 1;
> -    }
> -    else {
> -        /* a multiple range */
> -        num_ranges = 2;
> +    range += 6;
> +    or = apr_pstrdup(r->pool, range);
> +    merged = apr_pstrdup(r->pool, "");
> +    while ((cur = ap_getword(r->pool, &range, ','))) {
> +        char *dash;
> +        char *errp;
> +        apr_off_t number, start, end;
> +        
> +        if (!(dash = strchr(cur, '-'))) {
> +            break;
> +        }
> +        
> +        if (dash == cur) {
> +            /* In the form "-5"... leave as is */
> +            merged = apr_pstrcat(r->pool, merged, (num_ranges++ ? "," : ""), cur,
NULL);
> +            continue;
> +        }
> +        *dash++ = '\0';
> +        if (!*dash) {
> +            /* form "5-" */
> +            merged = apr_pstrcat(r->pool, merged, (num_ranges++ ? "," : ""), cur,
"-", NULL);
> +            continue;
> +        }
> +        /*
> +         * we have #-#, so we need to grab them... we don't bother
> +         * doing this for the #- or -# cases for speed reasons.
> +         * After all, those will be fixed when the filter parses
> +         * the merged range
> +         */
> +        if (apr_strtoff(&number, cur, &errp, 10) || *errp || number < 0)
{
> +            break;
> +        }
> +        start = number;
> +        if (apr_strtoff(&number, dash, &errp, 10) || *errp || number <= 0)
{
> +            break;
> +        }
> +        end = number;
> +        if (!in_merge) {
> +            ostart = start;
> +            oend = end;
> +        }
> +        in_merge = 0;
> +        if (start <= ostart) {
> +            ostart = start;
> +            in_merge = 1;
> +        }
> +        if (start > ostart && start < oend) {
> +            in_merge = 1;
> +        }
> +        if ((end-1) >= oend) {
> +            oend = end;
> +            in_merge = 1;
> +        }
> +        if (end > ostart && end < oend) {
> +            in_merge = 1;
> +        }
> +        if (in_merge) {
> +            continue;

I followed through the above algorithm with a range of "0-1,1000-1001" and the
result was a range of "0-1001" which feels wrong. Culprit seems to be the ((end-1) >= oend)
check.
Shouldn't that be oend >= (start-1)?

> +        } else {
> +            char *nr = apr_psprintf(r->pool, "%" APR_OFF_T_FMT "-%" APR_OFF_T_FMT,
> +                                    ostart, oend);
> +            merged = apr_pstrcat(r->pool, merged, (num_ranges++ ? "," : ""), nr,
NULL);

Doing this in a loop feels bad as it creates new buffers over and over.
Shouldn't we create a buffer of the size of strlen(range) and add to this subsequently
(we know that the length of the merged range <= length of range)?

> +        }
>      }
>  
> +    if (in_merge) {
> +        char *nr = apr_psprintf(r->pool, "%" APR_OFF_T_FMT "-%" APR_OFF_T_FMT,
> +                                ostart, oend);
> +        merged = apr_pstrcat(r->pool, merged, (num_ranges++ ? "," : ""), nr, NULL);
> +    }
> +        
>      r->status = HTTP_PARTIAL_CONTENT;
> -    r->range = range + 6;
> +    r->range = merged;
> +    ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
> +                  "Range: %s | %s", or, merged);
>  
>      return num_ranges;
>  }
> 


Regards

RĂ¼diger

Mime
View raw message