| Message ID | 20171229220606.29155-2-ffmpeg@tmm1.net |
|---|---|
| State | New |
| Headers | show |
Aman Gupta (2017-12-29): > From: Aman Gupta <aman@tmm1.net> > > This fixes a segfault when streaming from an HLS playlist which uses > crypto, by ensuring ff_http_do_new_request will never be invoked on a > non-http URLContext. > > Additionally it simplifies the demuxer by removing http.h and all the > http specific code in open_url_keepalive. It it awfully specific for something quite minor. And it is messing with the public API, that requires careful design. I suggest to have ff_http_do_new_request() check that its argument is really a HTTP context and return EINVAL or ENOSYS otherwise. Regards,
On Fri, Dec 29, 2017 at 2:45 PM, Nicolas George <george@nsup.org> wrote: > Aman Gupta (2017-12-29): > > From: Aman Gupta <aman@tmm1.net> > > > > This fixes a segfault when streaming from an HLS playlist which uses > > crypto, by ensuring ff_http_do_new_request will never be invoked on a > > non-http URLContext. > > > > Additionally it simplifies the demuxer by removing http.h and all the > > http specific code in open_url_keepalive. > > It it awfully specific for something quite minor. And it is messing with > the public API, that requires careful design. > I understand the public API should not be changed lightly, but I believe this approach is far superior to the mess of http specific checks currently present all over hls.c. It also makes it easier in the future to add http keepalive support to other consumers like the dash demuxer and the crypto protocol. > > I suggest to have ff_http_do_new_request() check that its argument is > really a HTTP context and return EINVAL or ENOSYS otherwise. > I'll go ahead and submit another patchset using this approach to fix the immediate segfault while we consider a larger API change. Thanks, Aman > > Regards, > > -- > Nicolas George > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >
On Fri, 29 Dec 2017 23:45:10 +0100 Nicolas George <george@nsup.org> wrote: > Aman Gupta (2017-12-29): > > From: Aman Gupta <aman@tmm1.net> > > > > This fixes a segfault when streaming from an HLS playlist which uses > > crypto, by ensuring ff_http_do_new_request will never be invoked on a > > non-http URLContext. > > > > Additionally it simplifies the demuxer by removing http.h and all the > > http specific code in open_url_keepalive. > > It it awfully specific for something quite minor. And it is messing with > the public API, that requires careful design. Reusing a http connection is not minor. It's pretty straightforward too, and doesn't require being stuck forever in a bikeshed carousel. > I suggest to have ff_http_do_new_request() check that its argument is > really a HTTP context and return EINVAL or ENOSYS otherwise. Unclean and fragile. How many more bugs do you want to have with this?
Aman Gupta (2017-12-29): > It also makes it easier in the future to add http keepalive support to > other consumers like the dash demuxer and the crypto protocol. For that, the API does not need to be public, it just need to be clean. I thought you were referring to using the hls demuxer with other protocols that may allow connection reuse. That could justify a public API. But we do not know what kind of parameters these other protocols may need; maybe some of them will require flags to select parallel / sequential requests; maybe one of them will need to return some kind of connection handle. We do not know, therefore we need to plan carefully, or even better: wait for an actual case. Also, naming the callback http_something would be a misnomer. > I'll go ahead and submit another patchset using this approach to fix > the immediate segfault while we consider a larger API change. Thanks. It seems to me a very sane course of action, and I have no objection to the two patches you just sent. Regards,
On Sat, 30 Dec 2017 11:53:14 +0100 Nicolas George <george@nsup.org> wrote: > Aman Gupta (2017-12-29): > > It also makes it easier in the future to add http keepalive support to > > other consumers like the dash demuxer and the crypto protocol. > > For that, the API does not need to be public, it just need to be clean. > > I thought you were referring to using the hls demuxer with other > protocols that may allow connection reuse. That could justify a public > API. But we do not know what kind of parameters these other protocols > may need; maybe some of them will require flags to select parallel / > sequential requests; maybe one of them will need to return some kind of > connection handle. We do not know, therefore we need to plan carefully, > or even better: wait for an actual case. Also, naming the callback > http_something would be a misnomer. As usual, you block patches based on some theoretical nonsense that will never happen anyway. Like your XML parser. > > I'll go ahead and submit another patchset using this approach to fix > > the immediate segfault while we consider a larger API change. > > Thanks. It seems to me a very sane course of action, and I have no > objection to the two patches you just sent. Well I object to the other patches.
diff --git a/libavformat/hls.c b/libavformat/hls.c index dccc7c7dd2..6c638537ff 100644 --- a/libavformat/hls.c +++ b/libavformat/hls.c @@ -26,7 +26,6 @@ * http://tools.ietf.org/html/draft-pantos-http-live-streaming */ -#include "libavformat/http.h" #include "libavutil/avstring.h" #include "libavutil/avassert.h" #include "libavutil/intreadwrite.h" @@ -611,19 +610,13 @@ static void update_options(char **dest, const char *name, void *src) static int open_url_keepalive(AVFormatContext *s, AVIOContext **pb, const char *url) { -#if !CONFIG_HTTP_PROTOCOL - return AVERROR_PROTOCOL_NOT_FOUND; -#else int ret; - URLContext *uc = ffio_geturlcontext(*pb); - av_assert0(uc); (*pb)->eof_reached = 0; - ret = ff_http_do_new_request(uc, url); + ret = (*pb)->new_http_request(*pb, url); if (ret < 0) { ff_format_io_close(s, pb); } return ret; -#endif } static int open_url(AVFormatContext *s, AVIOContext **pb, const char *url, @@ -670,7 +663,7 @@ static int open_url(AVFormatContext *s, AVIOContext **pb, const char *url, else if (strcmp(proto_name, "file") || !strncmp(url, "file,", 5)) return AVERROR_INVALIDDATA; - if (is_http && c->http_persistent && *pb) { + if (c->http_persistent && *pb && (*pb)->new_http_request) { ret = open_url_keepalive(c->ctx, pb, url); if (ret == AVERROR_EXIT) { return ret; @@ -725,9 +718,9 @@ static int parse_playlist(HLSContext *c, const char *url, struct variant_info variant_info; char tmp_str[MAX_URL_SIZE]; struct segment *cur_init_section = NULL; - int is_http = av_strstart(url, "http", NULL); - if (is_http && !in && c->http_persistent && c->playlist_pb) { + if (!in && c->http_persistent && + c->playlist_pb && c->playlist_pb->new_http_request) { in = c->playlist_pb; ret = open_url_keepalive(c->ctx, &c->playlist_pb, url); if (ret == AVERROR_EXIT) { @@ -761,7 +754,7 @@ static int parse_playlist(HLSContext *c, const char *url, if (ret < 0) return ret; - if (is_http && c->http_persistent) + if (c->http_persistent && in && in->new_http_request) c->playlist_pb = in; else close_in = 1; @@ -1511,7 +1504,7 @@ reload: return ret; } - if (c->http_persistent && av_strstart(seg->url, "http", NULL)) { + if (c->http_persistent && v->input && v->input->new_http_request) { v->input_read_done = 1; } else { ff_format_io_close(v->parent, &v->input);