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.
>
> 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;
> +
> + 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;
> + }
> +
> + 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.
