Message ID | 82AD7217-05B8-4D7F-859C-B271BD47EB70@me.com |
---|---|
State | Accepted |
Commit | 8c8e5d5286bf598a89ef9993a2cf6ea409d03a32 |
Headers | show |
On Thu, Jan 26, 2017 at 10:44:33AM -0600, Joel Cunningham wrote: > Michael, > > Thanks for the review and testing! New patch included, see comments below > > > > > this segfaults with many fuzzed files > > backtrace looks like this: > > > > #0 0x00007fffffffb368 in ?? () > > #1 0x00000000005f9280 in avio_seek (s=0x7fffffffb278, offset=31, whence=0) at libavformat/aviobuf.c:259 > > > > I found I wasn’t setting s->short_seek_get to NULL in ffio_init_context() and was relying on the zeroing aspect of av_mallocz() in avio_alloc_context(). From a grep of the source, it looks like fifo_init_context() can be called from other functions without having zero’d the AVIOContext first. > > Is the fuzz test exercising the AVIOContext in this manner? I’d also like to reproduce this test if there are instructions > > >> > >> diff --git a/libavformat/avio.c b/libavformat/avio.c > >> index 3606eb0..ecf6801 100644 > >> --- a/libavformat/avio.c > >> +++ b/libavformat/avio.c > >> @@ -645,6 +645,13 @@ int ffurl_get_multi_file_handle(URLContext *h, int **handles, int *numhandles) > >> return h->prot->url_get_multi_file_handle(h, handles, numhandles); > >> } > >> > >> +int ffurl_get_short_seek(URLContext *h) > >> +{ > >> + if (!h->prot->url_get_short_seek) > > > >> + return -1; > > > > should be some AVERROR code > > Fixed > > >> + return h->prot->url_get_short_seek(h); > >> +} > >> + > >> int ffurl_shutdown(URLContext *h, int flags) > >> { > >> if (!h->prot->url_shutdown) > >> diff --git a/libavformat/avio.h b/libavformat/avio.h > >> index b1ce1d1..0480981 100644 > >> --- a/libavformat/avio.h > >> +++ b/libavformat/avio.h > >> @@ -287,6 +287,12 @@ typedef struct AVIOContext { > >> int short_seek_threshold; > >> > >> /** > >> + * A callback that is used instead of short_seek_threshold. > >> + * This is current internal only, do not use from outside. > >> + */ > >> + int (*short_seek_get)(void *opaque); > >> + > >> + /** > >> * ',' separated list of allowed protocols. > >> */ > >> const char *protocol_whitelist; > > > > thats not ok to add this way, the docs say this: > > /** > > * Bytestream IO Context. > > * New fields can be added to the end with minor version bumps. > > * Removal, reordering and changes to existing fields require a major > > * version bump. > > * sizeof(AVIOContext) must not be used outside libav*. > > * > > * @note None of the function pointers in AVIOContext should be called > > * directly, they should only be set by the client application > > * when implementing custom I/O. Normally these are set to the > > * function pointers specified in avio_alloc_context() > > */ > > > > I moved short_seek_get to the end of AVIOContext. I’m not sure if the minor version bump referenced in the comment requires any in-code changes. Is this an acceptable magnitude of change for this feature? > > > [...] > >> --- a/libavformat/tcp.c > >> +++ b/libavformat/tcp.c > >> @@ -265,6 +265,26 @@ static int tcp_get_file_handle(URLContext *h) > >> return s->fd; > >> } > >> > >> +static int tcp_get_window_size(URLContext *h) > >> +{ > >> + TCPContext *s = h->priv_data; > >> + int avail; > >> + int avail_len = sizeof(avail); > >> + > > > >> + #if HAVE_WINSOCK2_H > > > > this formating is IIRC not allowed in C the # must be in the first > > column > > > > > >> + /* SO_RCVBUF with winsock only reports the actual TCP window size when > >> + auto-tuning has been disabled via setting SO_RCVBUF */ > >> + if (s->recv_buffer_size < 0) { > >> + return AVERROR(ENOSYS); > >> + } > >> + #endif > >> + > >> + if (getsockopt(s->fd, SOL_SOCKET, SO_RCVBUF, &avail, &avail_len)) { > >> + return ff_neterrno(); > >> + } > > > > the indention is inconsisntent > > Both formatting issues have been corrected > > From bac0f351afd14c56c56376d20557b7330bcea23e Mon Sep 17 00:00:00 2001 > From: Joel Cunningham <joel.cunningham@me.com> > Date: Fri, 13 Jan 2017 10:52:25 -0600 > Subject: [PATCH] HTTP: improve performance by reducing forward seeks please attach a proper git format-patch or use git send-email applying this is not as smooth and easy as it should be [...]
Attached is a git format-patch file > On Jan 26, 2017, at 6:52 PM, Michael Niedermayer <michaelni@gmx.at> wrote: > > On Thu, Jan 26, 2017 at 10:44:33AM -0600, Joel Cunningham wrote: >> Michael, >> >> Thanks for the review and testing! New patch included, see comments below >> >>> >>> this segfaults with many fuzzed files >>> backtrace looks like this: >>> >>> #0 0x00007fffffffb368 in ?? () >>> #1 0x00000000005f9280 in avio_seek (s=0x7fffffffb278, offset=31, whence=0) at libavformat/aviobuf.c:259 >>> >> >> I found I wasn’t setting s->short_seek_get to NULL in ffio_init_context() and was relying on the zeroing aspect of av_mallocz() in avio_alloc_context(). From a grep of the source, it looks like fifo_init_context() can be called from other functions without having zero’d the AVIOContext first. >> >> Is the fuzz test exercising the AVIOContext in this manner? I’d also like to reproduce this test if there are instructions >> >>>> >>>> diff --git a/libavformat/avio.c b/libavformat/avio.c >>>> index 3606eb0..ecf6801 100644 >>>> --- a/libavformat/avio.c >>>> +++ b/libavformat/avio.c >>>> @@ -645,6 +645,13 @@ int ffurl_get_multi_file_handle(URLContext *h, int **handles, int *numhandles) >>>> return h->prot->url_get_multi_file_handle(h, handles, numhandles); >>>> } >>>> >>>> +int ffurl_get_short_seek(URLContext *h) >>>> +{ >>>> + if (!h->prot->url_get_short_seek) >>> >>>> + return -1; >>> >>> should be some AVERROR code >> >> Fixed >> >>>> + return h->prot->url_get_short_seek(h); >>>> +} >>>> + >>>> int ffurl_shutdown(URLContext *h, int flags) >>>> { >>>> if (!h->prot->url_shutdown) >>>> diff --git a/libavformat/avio.h b/libavformat/avio.h >>>> index b1ce1d1..0480981 100644 >>>> --- a/libavformat/avio.h >>>> +++ b/libavformat/avio.h >>>> @@ -287,6 +287,12 @@ typedef struct AVIOContext { >>>> int short_seek_threshold; >>>> >>>> /** >>>> + * A callback that is used instead of short_seek_threshold. >>>> + * This is current internal only, do not use from outside. >>>> + */ >>>> + int (*short_seek_get)(void *opaque); >>>> + >>>> + /** >>>> * ',' separated list of allowed protocols. >>>> */ >>>> const char *protocol_whitelist; >>> >>> thats not ok to add this way, the docs say this: >>> /** >>> * Bytestream IO Context. >>> * New fields can be added to the end with minor version bumps. >>> * Removal, reordering and changes to existing fields require a major >>> * version bump. >>> * sizeof(AVIOContext) must not be used outside libav*. >>> * >>> * @note None of the function pointers in AVIOContext should be called >>> * directly, they should only be set by the client application >>> * when implementing custom I/O. Normally these are set to the >>> * function pointers specified in avio_alloc_context() >>> */ >>> >> >> I moved short_seek_get to the end of AVIOContext. I’m not sure if the minor version bump referenced in the comment requires any in-code changes. Is this an acceptable magnitude of change for this feature? >> >>> [...] >>>> --- a/libavformat/tcp.c >>>> +++ b/libavformat/tcp.c >>>> @@ -265,6 +265,26 @@ static int tcp_get_file_handle(URLContext *h) >>>> return s->fd; >>>> } >>>> >>>> +static int tcp_get_window_size(URLContext *h) >>>> +{ >>>> + TCPContext *s = h->priv_data; >>>> + int avail; >>>> + int avail_len = sizeof(avail); >>>> + >>> >>>> + #if HAVE_WINSOCK2_H >>> >>> this formating is IIRC not allowed in C the # must be in the first >>> column >>> >>> >>>> + /* SO_RCVBUF with winsock only reports the actual TCP window size when >>>> + auto-tuning has been disabled via setting SO_RCVBUF */ >>>> + if (s->recv_buffer_size < 0) { >>>> + return AVERROR(ENOSYS); >>>> + } >>>> + #endif >>>> + >>>> + if (getsockopt(s->fd, SOL_SOCKET, SO_RCVBUF, &avail, &avail_len)) { >>>> + return ff_neterrno(); >>>> + } >>> >>> the indention is inconsisntent >> >> Both formatting issues have been corrected >> >> From bac0f351afd14c56c56376d20557b7330bcea23e Mon Sep 17 00:00:00 2001 >> From: Joel Cunningham <joel.cunningham@me.com> >> Date: Fri, 13 Jan 2017 10:52:25 -0600 >> Subject: [PATCH] HTTP: improve performance by reducing forward seeks > > > please attach a proper git format-patch or use git send-email > > applying this is not as smooth and easy as it should be > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > If you fake or manipulate statistics in a paper in physics you will never > get a job again. > If you fake or manipulate statistics in a paper in medicin you will get > a job for life at the pharma industry. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff --git a/libavformat/avio.c b/libavformat/avio.c index 3606eb0..62233a6 100644 --- a/libavformat/avio.c +++ b/libavformat/avio.c @@ -645,6 +645,13 @@ int ffurl_get_multi_file_handle(URLContext *h, int **handles, int *numhandles) return h->prot->url_get_multi_file_handle(h, handles, numhandles); } +int ffurl_get_short_seek(URLContext *h) +{ + if (!h->prot->url_get_short_seek) + return AVERROR(ENOSYS); + return h->prot->url_get_short_seek(h); +} + int ffurl_shutdown(URLContext *h, int flags) { if (!h->prot->url_shutdown) diff --git a/libavformat/avio.h b/libavformat/avio.h index b1ce1d1..a088f05 100644 --- a/libavformat/avio.h +++ b/libavformat/avio.h @@ -313,6 +313,12 @@ typedef struct AVIOContext { */ enum AVIODataMarkerType current_type; int64_t last_time; + + /** + * A callback that is used instead of short_seek_threshold. + * This is current internal only, do not use from outside. + */ + int (*short_seek_get)(void *opaque); } AVIOContext; /** diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c index 134d627..b102510 100644 --- a/libavformat/aviobuf.c +++ b/libavformat/aviobuf.c @@ -119,6 +119,7 @@ int ffio_init_context(AVIOContext *s, s->ignore_boundary_point = 0; s->current_type = AVIO_DATA_MARKER_UNKNOWN; s->last_time = AV_NOPTS_VALUE; + s->short_seek_get = NULL; return 0; } @@ -233,6 +234,7 @@ int64_t avio_seek(AVIOContext *s, int64_t offset, int whence) int64_t pos; int force = whence & AVSEEK_FORCE; int buffer_size; + int short_seek; whence &= ~AVSEEK_FORCE; if(!s) @@ -254,13 +256,21 @@ int64_t avio_seek(AVIOContext *s, int64_t offset, int whence) if (offset < 0) return AVERROR(EINVAL); + if (s->short_seek_get) { + short_seek = s->short_seek_get(s->opaque); + /* fallback to default short seek */ + if (short_seek <= 0) + short_seek = s->short_seek_threshold; + } else + short_seek = s->short_seek_threshold; + offset1 = offset - pos; // "offset1" is the relative offset from the beginning of s->buffer if (!s->must_flush && (!s->direct || !s->seek) && offset1 >= 0 && offset1 <= buffer_size - s->write_flag) { /* can do the seek inside the buffer */ s->buf_ptr = s->buffer + offset1; } else if ((!s->seekable || - offset1 <= buffer_size + s->short_seek_threshold) && + offset1 <= buffer_size + short_seek) && !s->write_flag && offset1 >= 0 && (!s->direct || !s->seek) && (whence != SEEK_END || force)) { @@ -858,6 +868,12 @@ static int64_t io_seek(void *opaque, int64_t offset, int whence) return ffurl_seek(internal->h, offset, whence); } +static int io_short_seek(void *opaque) +{ + AVIOInternal *internal = opaque; + return ffurl_get_short_seek(internal->h); +} + static int io_read_pause(void *opaque, int pause) { AVIOInternal *internal = opaque; @@ -919,6 +935,7 @@ int ffio_fdopen(AVIOContext **s, URLContext *h) (*s)->read_pause = io_read_pause; (*s)->read_seek = io_read_seek; } + (*s)->short_seek_get = io_short_seek; (*s)->av_class = &ff_avio_class; return 0; fail: diff --git a/libavformat/http.c b/libavformat/http.c index 944a6cf..b354c87 100644 --- a/libavformat/http.c +++ b/libavformat/http.c @@ -1524,6 +1524,12 @@ static int http_get_file_handle(URLContext *h) return ffurl_get_file_handle(s->hd); } +static int http_get_short_seek(URLContext *h) +{ + HTTPContext *s = h->priv_data; + return ffurl_get_short_seek(s->hd); +} + #define HTTP_CLASS(flavor) \ static const AVClass flavor ## _context_class = { \ .class_name = # flavor, \ @@ -1545,6 +1551,7 @@ const URLProtocol ff_http_protocol = { .url_seek = http_seek, .url_close = http_close, .url_get_file_handle = http_get_file_handle, + .url_get_short_seek = http_get_short_seek, .url_shutdown = http_shutdown, .priv_data_size = sizeof(HTTPContext), .priv_data_class = &http_context_class, @@ -1564,6 +1571,7 @@ const URLProtocol ff_https_protocol = { .url_seek = http_seek, .url_close = http_close, .url_get_file_handle = http_get_file_handle, + .url_get_short_seek = http_get_short_seek, .url_shutdown = http_shutdown, .priv_data_size = sizeof(HTTPContext), .priv_data_class = &https_context_class, diff --git a/libavformat/tcp.c b/libavformat/tcp.c index 25abafc..bfed5f5 100644 --- a/libavformat/tcp.c +++ b/libavformat/tcp.c @@ -265,6 +265,26 @@ static int tcp_get_file_handle(URLContext *h) return s->fd; } +static int tcp_get_window_size(URLContext *h) +{ + TCPContext *s = h->priv_data; + int avail; + int avail_len = sizeof(avail); + +#if HAVE_WINSOCK2_H + /* SO_RCVBUF with winsock only reports the actual TCP window size when + auto-tuning has been disabled via setting SO_RCVBUF */ + if (s->recv_buffer_size < 0) { + return AVERROR(ENOSYS); + } +#endif + + if (getsockopt(s->fd, SOL_SOCKET, SO_RCVBUF, &avail, &avail_len)) { + return ff_neterrno(); + } + return avail; +} + const URLProtocol ff_tcp_protocol = { .name = "tcp", .url_open = tcp_open, @@ -273,6 +293,7 @@ const URLProtocol ff_tcp_protocol = { .url_write = tcp_write, .url_close = tcp_close, .url_get_file_handle = tcp_get_file_handle, + .url_get_short_seek = tcp_get_window_size, .url_shutdown = tcp_shutdown, .priv_data_size = sizeof(TCPContext), .flags = URL_PROTOCOL_FLAG_NETWORK, diff --git a/libavformat/url.h b/libavformat/url.h index 5c50245..910f1e0 100644 --- a/libavformat/url.h +++ b/libavformat/url.h @@ -84,6 +84,7 @@ typedef struct URLProtocol { int (*url_get_file_handle)(URLContext *h); int (*url_get_multi_file_handle)(URLContext *h, int **handles, int *numhandles); + int (*url_get_short_seek)(URLContext *h); int (*url_shutdown)(URLContext *h, int flags); int priv_data_size; const AVClass *priv_data_class; @@ -249,6 +250,13 @@ int ffurl_get_file_handle(URLContext *h); int ffurl_get_multi_file_handle(URLContext *h, int **handles, int *numhandles); /** + * Return the current short seek threshold value for this URL. + * + * @return threshold (>0) on success or <=0 on error. + */ +int ffurl_get_short_seek(URLContext *h); + +/** * Signal the URLContext that we are done reading or writing the stream. * * @param h pointer to the resource