Florian Obser(flor...@openbsd.org) on 2015.05.03 12:39:02 +0000:
> On Sun, May 03, 2015 at 01:46:56PM +0200, Sunil Nimmagadda wrote:
> > On Sat, May 02, 2015 at 02:49:30PM +0000, Florian Obser wrote:
> > > Sorry for the very late reply, I'm currently very busy :/
> >
> > Thank you for taking time to review it. A new patch with style nits
> > fixed and a gratuitous NULL check removed.
> >
> > [trimming some text]
> >
> > > this is missing the server_file_method() song and dance from
> > > server_file_request()
> >
> > Fixed in a slightly different way than using server_file_method().
> > Since range request should be ignored for methods other than GET,
> > fallback to server_file_request() for further processing/validation.
>
> yep, this is good.
>
> >
> > > > + if ((range = parse_range(range_str, st->st_size, &nranges)) ==
> > > > NULL) {
> > > > + code = 416;
> > > > + (void)snprintf(content_range, sizeof(content_range),
> > > > + "bytes */%lld", st->st_size);
> > > > + errstr = content_range;
> > > > + goto abort;
> > > > + }
> > >
> > > hm, apache answers with the full file and 200 if the range header is
> > > syntactically incorrect or if end < start. As far as I can tell it
> > > only answers 416 if the range actually lies outside of the file.
> >
> > I am confused about the RFC here, section 4.4 states that 416 is
> > returned if "...the set of ranges requested has been rejected due
> > to invalid ranges..." and a note follows immediately, "Because
> > servers are free to ignore Range, many implementations will simply
> > respond with the entire selected representation in a 200 response"
> >
> > As you noted apache returns 200 while nginx returns 416 for an
> > incorrect range. What should httpd do ideally?
>
> Thanks for checking nginx, I don't have one around.
> I noted this for 2 reasons:
> 1) I guess apache is the defacto standard, so if it behaves
> differently there might be a reason, maybe broken clients in the wild.
> 2) The way the code is currently structured it will require a bit of
> shuffling around if we ever want apache's behaviour.
>
> That being said, my understanding of the RFC is that your
> implementation is correct. And with nginx behaving this way, too, it
> shouldn't bite us in the future. :)
>
> >
> > > > +
> > > > + /* Accept byte ranges */
> > > > + if (code == 200 &&
> > > > + kv_add(&resp->http_headers, "Accept-Ranges", "bytes") ==
> > > > NULL)
> > > > return (-1);
> > >
> > > I don't think we should advertise ranges for all 200 results, for
> > > example we don't support it for directory indexes.
> >
> > Agree, since RFC says "server MAY send Accept-Range header" and
> > "clients MAY generate range requests without having received this
> > header field", dropping this header shouldn't make a difference.
> >
> > Comments?
>
> This looks good.
>
> OK florian@ if someone wants to commit it. Or give me an OK and I'll
> commit.
>
fwiw i dont know much about ranges, but this reads ok. two whitespace bits
below. otherwise ok benno.
one question though: whats the reasoning behind MAX_RANGES 4? nginx seems to
have a default of "unlimited" (which i think questionable), but what is
reasonably seen on the internet?
> >
> > Index: server_file.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/httpd/server_file.c,v
> > retrieving revision 1.52
> > diff -u -p -r1.52 server_file.c
> > --- server_file.c 25 Apr 2015 14:40:35 -0000 1.52
> > +++ server_file.c 3 May 2015 11:18:07 -0000
> > @@ -36,12 +36,25 @@
> >
> > #define MINIMUM(a, b) (((a) < (b)) ? (a) : (b))
> > #define MAXIMUM(a, b) (((a) > (b)) ? (a) : (b))
> > +#define MAX_RANGES 4
> >
> > -int server_file_access(struct httpd *, struct client *, char *,
> > size_t);
> > -int server_file_request(struct httpd *, struct client *, char *,
> > - struct stat *);
> > -int server_file_index(struct httpd *, struct client *, struct stat
> > *);
> > -int server_file_method(struct client *);
> > +struct range {
> > + off_t start;
> > + off_t end;
> > +};
> > +
> > +int server_file_access(struct httpd *, struct client *,
> > + char *, size_t);
> > +int server_file_request(struct httpd *, struct client *,
> > + char *, struct stat *);
> > +int server_partial_file_request(struct httpd *, struct
> > client *,
> > + char *, struct stat *, char *);
> > +int server_file_index(struct httpd *, struct client *,
> > + struct stat *);
> > +int server_file_method(struct client *);
> > +int parse_range_spec(char *, size_t, struct range *);
> > +struct range *parse_range(char *, size_t, int *);
> > +int buffer_add_range(int, struct evbuffer *, struct range
> > *);
> >
> > int
> > server_file_access(struct httpd *env, struct client *clt,
> > @@ -50,6 +63,7 @@ server_file_access(struct httpd *env, st
> > struct http_descriptor *desc = clt->clt_descreq;
> > struct server_config *srv_conf = clt->clt_srv_conf;
> > struct stat st;
> > + struct kv *r, key;
> > char *newpath;
> > int ret;
> >
> > @@ -123,7 +137,13 @@ server_file_access(struct httpd *env, st
> > goto fail;
> > }
> >
> > - return (server_file_request(env, clt, path, &st));
> > + key.kv_key = "Range";
> > + r = kv_find(&desc->http_headers, &key);
> > + if (r != NULL)
> > + return (server_partial_file_request(env, clt, path, &st,
> > + r->kv_value));
> > + else
> > + return (server_file_request(env, clt, path, &st));
> >
> > fail:
> > switch (errno) {
> > @@ -262,6 +282,143 @@ server_file_request(struct httpd *env, s
> > }
> >
> > int
> > +server_partial_file_request(struct httpd *env, struct client *clt, char
> > *path,
> > + struct stat *st, char *range_str)
> > +{
> > + struct http_descriptor *resp = clt->clt_descresp;
> > + struct http_descriptor *desc = clt->clt_descreq;
> > + struct media_type *media, multipart_media;
> > + struct range *range;
> > + struct evbuffer *evb = NULL;
> > + size_t content_length;
> > + int code = 500, fd = -1, i, nranges, ret;
> > + uint32_t boundary;
> > + char content_range[64];
> > + const char *errstr = NULL;
> > +
> > + /* Ignore range request for methods other than GET */
> > + if (desc->http_method != HTTP_METHOD_GET)
> > + return server_file_request(env, clt, path, st);
> > +
> > + if ((range = parse_range(range_str, st->st_size, &nranges)) == NULL) {
> > + code = 416;
> > + (void)snprintf(content_range, sizeof(content_range),
> > + "bytes */%lld", st->st_size);
> > + errstr = content_range;
> > + goto abort;
> > + }
> > +
> > + /* Now open the file, should be readable or we have another problem */
> > + if ((fd = open(path, O_RDONLY)) == -1)
> > + goto abort;
> > +
> > + media = media_find(env->sc_mediatypes, path);
> > + if ((evb = evbuffer_new()) == NULL) {
> > + errstr = "failed to allocate file buffer";
> > + goto abort;
> > + }
> > +
> > + if (nranges == 1) {
> > + (void)snprintf(content_range, sizeof(content_range),
> > + "bytes %lld-%lld/%lld", range->start, range->end,
> > + st->st_size);
> > + if (kv_add(&resp->http_headers, "Content-Range",
> > + content_range) == NULL)
> > + goto abort;
> > +
> > + content_length = range->end - range->start + 1;
> > + if (buffer_add_range(fd, evb, range) == 0)
> > + goto abort;
> > +
> > + } else {
> > + content_length = 0;
> > + boundary = arc4random();
> > + /* Generate a multipart payload of byteranges */
> > + while (nranges--) {
> > + if ((i = evbuffer_add_printf(evb, "\r\n--%ud\r\n",
> > + boundary)) == -1)
> > + goto abort;
> > +
> > + content_length += i;
> > + if ((i = evbuffer_add_printf(evb,
> > + "Content-Type: %s/%s\r\n",
> > + media == NULL ? "application" : media->media_type,
> > + media == NULL ?
> > + "octet-stream" : media->media_subtype)) == -1)
> > + goto abort;
> > +
> > + content_length += i;
> > + if ((i = evbuffer_add_printf(evb,
> > + "Content-Range: bytes %lld-%lld/%lld\r\n\r\n",
> > + range->start, range->end, st->st_size)) == -1)
> > + goto abort;
> > +
whitespace on blan line
> > + content_length += i;
> > + if (buffer_add_range(fd, evb, range) == 0)
> > + goto abort;
> > +
> > + content_length += range->end - range->start + 1;
> > + range++;
> > + }
> > +
> > + if ((i = evbuffer_add_printf(evb, "\r\n--%ud--\r\n",
> > + boundary)) == -1)
> > + goto abort;
> > +
> > + content_length += i;
> > +
> > + /* prepare multipart/byteranges media type */
> > + (void)strlcpy(multipart_media.media_type, "multipart",
> > + sizeof(multipart_media.media_type));
> > + (void)snprintf(multipart_media.media_subtype,
> > + sizeof(multipart_media.media_subtype),
> > + "byteranges; boundary=%ud", boundary);
> > + media = &multipart_media;
> > + }
> > +
> > + ret = server_response_http(clt, 206, media, content_length,
> > + MINIMUM(time(NULL), st->st_mtim.tv_sec));
> > + switch (ret) {
> > + case -1:
> > + goto fail;
> > + case 0:
> > + /* Connection is already finished */
> > + close(fd);
> > + evbuffer_free(evb);
> > + evb = NULL;
> > + goto done;
> > + default:
> > + break;
> > + }
> > +
> > + if (server_bufferevent_write_buffer(clt, evb) == -1)
> > + goto fail;
> > +
> > + evbuffer_free(evb);
> > + evb = NULL;
> > +
> > + bufferevent_enable(clt->clt_bev, EV_READ|EV_WRITE);
> > + if (clt->clt_persist)
> > + clt->clt_toread = TOREAD_HTTP_HEADER;
> > + else
> > + clt->clt_toread = TOREAD_HTTP_NONE;
> > + clt->clt_done = 0;
> > +
> > + done:
> > + server_reset_http(clt);
> > + return (0);
> > + fail:
> > + bufferevent_disable(clt->clt_bev, EV_READ|EV_WRITE);
> > + bufferevent_free(clt->clt_bev);
> > + clt->clt_bev = NULL;
> > + abort:
> > + if (errstr == NULL)
> > + errstr = strerror(errno);
> > + server_abort_http(clt, code, errstr);
> > + return (-1);
> > +}
> > +
> > +int
> > server_file_index(struct httpd *env, struct client *clt, struct stat *st)
> > {
> > char path[PATH_MAX];
> > @@ -469,3 +626,112 @@ server_file_error(struct bufferevent *be
> > server_close(clt, "unknown event error");
> > return;
> > }
> > +
> > +struct range *
> > +parse_range(char *str, size_t file_sz, int *nranges)
> > +{
> > + static struct range ranges[MAX_RANGES];
> > + int i = 0;
> > + char *p, *q;
> > +
> > + /* Extract range unit */
> > + if ((p = strchr(str, '=')) == NULL)
> > + return (NULL);
> > +
> > + *p++ = '\0';
> > + /* Check if it's a bytes range spec */
> > + if (strcmp(str, "bytes") != 0)
> > + return (NULL);
> > +
> > + while ((q = strchr(p, ',')) != NULL) {
> > + *q++ = '\0';
> > +
> > + /* Extract start and end positions */
> > + if (parse_range_spec(p, file_sz, &ranges[i]) == 0)
> > + continue;
> > +
> > + i++;
> > + if (i == MAX_RANGES)
> > + return (NULL);
> > +
> > + p = q;
> > + }
> > +
> > + if (parse_range_spec(p, file_sz, &ranges[i]) != 0)
> > + i++;
> > +
> > + *nranges = i;
> > + return (i ? ranges : NULL);
> > +}
> > +
> > +int
> > +parse_range_spec(char *str, size_t size, struct range *r)
> > +{
> > + size_t start_str_len, end_str_len;
> > + char *p, *start_str, *end_str;
> > + const char *errstr;
> > +
> > + if ((p = strchr(str, '-')) == NULL)
> > + return (0);
> > +
> > + *p++ = '\0';
> > + start_str = str;
> > + end_str = p;
> > + start_str_len = strlen(start_str);
> > + end_str_len = strlen(end_str);
> > +
> > + /* Either 'start' or 'end' is optional but not both */
> > + if ((start_str_len == 0) && (end_str_len == 0))
> > + return (0);
> > +
> > + if (end_str_len) {
> > + r->end = strtonum(end_str, 0, LLONG_MAX, &errstr);
> > + if (errstr)
> > + return (0);
> > +
> > + if ((size_t)r->end >= size)
> > + r->end = size - 1;
> > + } else
> > + r->end = size - 1;
> > +
> > + if (start_str_len) {
> > + r->start = strtonum(start_str, 0, LLONG_MAX, &errstr);
> > + if (errstr)
> > + return (0);
> > +
> > + if ((size_t)r->start >= size)
> > + return (0);
> > + } else {
> > + r->start = size - r->end;
> > + r->end = size - 1;
> > + }
> > +
> > + if (r->end < r->start)
> > + return (0);
> > +
> > + return (1);
> > +}
> > +
> > +int
> > +buffer_add_range(int fd, struct evbuffer *evb, struct range *range)
> > +{
> > + char buf[BUFSIZ];
> > + size_t n, range_sz;
> > + ssize_t nread;
> > +
> > + if (lseek(fd, range->start, SEEK_SET) == -1)
> > + return (0);
> > +
> > + range_sz = range->end - range->start + 1;
> > + while (range_sz) {
> > + n = MINIMUM(range_sz, sizeof(buf));
> > + if ((nread = read(fd, buf, n)) == -1)
> > + return (0);
> > +
> > + evbuffer_add(evb, buf, nread);
> > + range_sz -= nread;
> > + }
> > +
whitespace on blank line
> > + return (1);
> > +}
> > +
> > Index: server_http.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/httpd/server_http.c,v
> > retrieving revision 1.78
> > diff -u -p -r1.78 server_http.c
> > --- server_http.c 18 Apr 2015 09:27:54 -0000 1.78
> > +++ server_http.c 3 May 2015 11:18:07 -0000
> > @@ -788,6 +788,13 @@ server_abort_http(struct client *clt, u_
> > extraheader = NULL;
> > }
> > break;
> > + case 416:
> > + if (asprintf(&extraheader,
> > + "Content-Range: %s\r\n", msg) == -1) {
> > + code = 500;
> > + extraheader = NULL;
> > + }
> > + break;
> > default:
> > /*
> > * Do not send details of the error. Traditionally,
>
> --
> I'm not entirely sure you are real.
>
