Re: [FFmpeg-devel] [PATCH] avformat/hls: look for trailing GET headers with m3u8 extension check

[Leo Izen <leo.izen@gmail.com>]

On 5/13/23 10:54, Michael Niedermayer wrote:
> On Fri, May 12, 2023 at 04:26:22PM -0400, Leo Izen wrote:
>> After commit 6b1f68ccb04d791f0250e05687c346a99ff47ea1 we refuse to use
>> URLs of the form https://foo.bar/baz.m3u8?foo=bar because it fails the
>> file extension check. This commit strips the ?foo=bar at the end before
>> checking the file extension.
>>
>> Signed-off-by: Leo Izen <leo.izen@gmail.com>
>> ---
>>   libavformat/hls.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavformat/hls.c b/libavformat/hls.c
>> index 11e345b280..6a97cced17 100644
>> --- a/libavformat/hls.c
>> +++ b/libavformat/hls.c
>> @@ -2534,7 +2534,16 @@ static int hls_probe(const AVProbeData *p)
>>           strstr(p->buf, "#EXT-X-TARGETDURATION:") ||
>>           strstr(p->buf, "#EXT-X-MEDIA-SEQUENCE:")) {
>>   
>> -        if (!av_match_ext(p->filename, "m3u8,hls,m3u")) {
>> +        char *request_qmark = strchr(p->filename, '?');
>> +        int match_ext;
>> +
>> +        if (request_qmark)
>> +            *request_qmark = '\0';
>> +        match_ext = av_match_ext(p->filename, "m3u8,hls,m3u");
>> +        if (request_qmark)
>> +            *request_qmark = '?';
>> +
>> +        if (!match_ext) {
>>               av_log(NULL, AV_LOG_ERROR, "Not detecting m3u8/hls with non standard extension\n");
>>               return 0;
>>           }
> 
> the av_match_ext here matches the probe code
> all should be fixed. Also differences between local files and urls should
> be considered in extension extraction

If you're requiring that we check that a file is local before stripping 
tailing request headers, how would you check if a file is local? having 
a scheme:// is not sufficient to make that check, as file:// is a valid 
scheme. You could check for https?:// I suppose, but the spec doesn't 
actually require that HTTP be used (section 2):

    Data SHOULD be carried over HTTP [RFC7230], but,
    in general, a URI can specify any protocol that can reliably transfer
    the specified resource on demand.

Do note that your original patch is not spec-compliant. RFC 8216 section 
4 says the following:

    Each Playlist file MUST be identifiable either by the path component
    of its URI or by HTTP Content-Type.  In the first case, the path MUST
    end with either .m3u8 or .m3u.  In the second, the HTTP Content-Type
    MUST be "application/vnd.apple.mpegurl" or "audio/mpegurl".  Clients
    SHOULD refuse to parse Playlists that are not so identified.


This implies that (1) .hls is not a valid extension if that is being 
used, and (2) a valid HLS mimetype in a content-type header is 
sufficient to mark a file as HLS regardless of the extension used.

So the extension check should only be done for local files or files that 
don't serve an appropriate content-type header, should you wish to do 
the extension check.

However, HTTP streams are always safe (mpv marks them as such, for 
example) so the original patch is still a bit silly.

- Leo Izen (Traneptora / thebombzen)
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".