[FFmpeg-devel,2/2] avformat/hls: use AVIOContext new_http_request callback

Submitted by Aman Gupta on Dec. 29, 2017, 10:06 p.m.

Details

Message ID 20171229220606.29155-2-ffmpeg@tmm1.net
State New
Headers show

Commit Message

Aman Gupta Dec. 29, 2017, 10:06 p.m.
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.

Signed-off-by: Aman Gupta <aman@tmm1.net>
Signed-off-by: wm4 <nfxjfg@googlemail.com>
---
 libavformat/hls.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

Comments

Nicolas George Dec. 29, 2017, 10:45 p.m.
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,
Aman Gupta Dec. 29, 2017, 11:38 p.m.
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
>
>
wm4 Dec. 30, 2017, 9:09 a.m.
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?
Nicolas George Dec. 30, 2017, 10:53 a.m.
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,
wm4 Dec. 30, 2017, 11:02 a.m.
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.

Patch hide | download patch | download mbox

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);