diff mbox series

[FFmpeg-devel] fix:?==?utf-8?q? rtsp/transport parser should accept comma for parameters

Message ID 4d-5eac7000-7-385e7000@156260547
State New
Headers show
Series [FFmpeg-devel] fix:?==?utf-8?q? rtsp/transport parser should accept comma for parameters
Related show

Checks

Context Check Description
andriy/default pending
andriy/configure warning Failed to apply patch

Commit Message

abalam May 1, 2020, 6:52 p.m. UTC
I needed this fix to make [Shinobi](https://github.com/ShinobiCCTV/Shinobi) works with recent chinese cameras.

We can read that FFmpeg is perfectly following the [RTSP specs](https://tools.ietf.org/html/rfc2326#page-58) : 
`Transports are comma separated, listed in order of preference. Parameters may be added to each transport, separated by a semicolon.`
But considering:
- VLC is working smart with not perfect cameras ;
- RTSP module in FFmpeg is not programmed to handle more than one transport ; 
=> this fix could be useful to anyone

I also think this fix might be improved by a better scan of parameters if ffmpeg wants to handle more than one transport.

Details: 
I'm using [Shinobi](https://github.com/ShinobiCCTV/Shinobi) to record cameras rtsp streams
For several cameras, cheap chinese ones, VLC is working to get stream but FFmpeg always crashes after it receives the RTSP transports parameters available.

`Transport: RTP/AVP;unicast;mode=PLAY;source=192.168.66.164;client_port=11658-11659;server_port=40000-40001,ssrc=FFFFCCCC`

After debugging the source code, it appears that FFmpeg is not considering that a comma `,` before `ssrc` is a correct separator and think it's a second transport.
Then it crashes because RTSP don't want to accept more than one transport.

( This was closed PR https://github.com/FFmpeg/FFmpeg/pull/336 )

Thank you for having read,
Keep good work and have a nice day!

---
 libavformat/rtsp.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

--
2.21.1 (Apple Git-122.3)

Comments

Marton Balint May 1, 2020, 7:54 p.m. UTC | #1
On Fri, 1 May 2020, abalam wrote:

>
> I needed this fix to make [Shinobi](https://github.com/ShinobiCCTV/Shinobi) works with recent chinese cameras.
>
> We can read that FFmpeg is perfectly following the [RTSP specs](https://tools.ietf.org/html/rfc2326#page-58) : 
> `Transports are comma separated, listed in order of preference. Parameters may be added to each transport, separated by a semicolon.`
> But considering:
> - VLC is working smart with not perfect cameras ;
> - RTSP module in FFmpeg is not programmed to handle more than one transport ; 
> => this fix could be useful to anyone
>
> I also think this fix might be improved by a better scan of parameters if ffmpeg wants to handle more than one transport.
>
> Details: 
> I'm using [Shinobi](https://github.com/ShinobiCCTV/Shinobi) to record cameras rtsp streams
> For several cameras, cheap chinese ones, VLC is working to get stream but FFmpeg always crashes after it receives the RTSP transports parameters available.
>
> `Transport: RTP/AVP;unicast;mode=PLAY;source=192.168.66.164;client_port=11658-11659;server_port=40000-40001,ssrc=FFFFCCCC`
>
> After debugging the source code, it appears that FFmpeg is not considering that a comma `,` before `ssrc` is a correct separator and think it's a second transport.
> Then it crashes because RTSP don't want to accept more than one transport.
>
> ( This was closed PR https://github.com/FFmpeg/FFmpeg/pull/336 )
>
> Thank you for having read,
> Keep good work and have a nice day!

I am not really familiar with RTSP code, but at least the parse function 
does seem to parse multiple transports. So effectively breaking it does 
not seem right. Maybe you should check out how VLC detects such broken 
devices, and implement something similar here, because probably VLC's 
workaround is proven to not cause valid transport specifications to be 
misinterpreted as a single transport.

Regards,
Marton

>
> ---
>  libavformat/rtsp.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
> index b2b3f32011..f77de10119 100644
> --- a/libavformat/rtsp.c
> +++ b/libavformat/rtsp.c
> @@ -1001,11 +1001,9 @@ static void rtsp_parse_transport(AVFormatContext *s,
>
>              while (*p != ';' && *p != '\0' && *p != ',')
>                  p++;
> -            if (*p == ';')
> +            if (*p == ';' || *p == ',')
>                  p++;
>          }
> -        if (*p == ',')
> -            p++;
>
>          reply->nb_transports++;
>          if (reply->nb_transports >= RTSP_MAX_TRANSPORTS)
> --
> 2.21.1 (Apple Git-122.3)
> _______________________________________________
> 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".
abalam May 2, 2020, 9:29 a.m. UTC | #2
Thank you for your answer Marton.
Yes I broke the parse process because I saw below in rtsp.c a specific source code which specify to have only "One transport" :
 ff_rtsp_send_cmd(s, "SETUP", rtsp_st->control_url, cmd, reply, NULL);
if (reply->status_code == 461 /* Unsupported protocol */ && i == 0) {
err = 1;
goto fail;
} else if (reply->status_code != RTSP_STATUS_OK ||
reply->nb_transports != 1) {
err = ff_rtsp_averror(reply->status_code, AVERROR_INVALIDDATA);
goto fail;
}

Maybe I should have take time to find out or ask here why there is a such condition "reply->nb_transports != 1"...

I'll check the VLC source code as soon as I find enough time... it's not easy with children around those COVID times.... :-/

Yann

On Friday, May 01, 2020 21:54 CEST, Marton Balint <cus@passwd.hu> wrote:
 

On Fri, 1 May 2020, abalam wrote:

>
> I needed this fix to make [Shinobi](https://github.com/ShinobiCCTV/Shinobi) works with recent chinese cameras.
>
> We can read that FFmpeg is perfectly following the [RTSP specs](https://tools.ietf.org/html/rfc2326#page-58) : 
> `Transports are comma separated, listed in order of preference. Parameters may be added to each transport, separated by a semicolon.`
> But considering:
> - VLC is working smart with not perfect cameras ;
> - RTSP module in FFmpeg is not programmed to handle more than one transport ; 
> => this fix could be useful to anyone
>
> I also think this fix might be improved by a better scan of parameters if ffmpeg wants to handle more than one transport.
>
> Details: 
> I'm using [Shinobi](https://github.com/ShinobiCCTV/Shinobi) to record cameras rtsp streams
> For several cameras, cheap chinese ones, VLC is working to get stream but FFmpeg always crashes after it receives the RTSP transports parameters available.
>
> `Transport: RTP/AVP;unicast;mode=PLAY;source=192.168.66.164;client_port=11658-11659;server_port=40000-40001,ssrc=FFFFCCCC`
>
> After debugging the source code, it appears that FFmpeg is not considering that a comma `,` before `ssrc` is a correct separator and think it's a second transport.
> Then it crashes because RTSP don't want to accept more than one transport.
>
> ( This was closed PR https://github.com/FFmpeg/FFmpeg/pull/336 )
>
> Thank you for having read,
> Keep good work and have a nice day!

I am not really familiar with RTSP code, but at least the parse function
does seem to parse multiple transports. So effectively breaking it does
not seem right. Maybe you should check out how VLC detects such broken
devices, and implement something similar here, because probably VLC's
workaround is proven to not cause valid transport specifications to be
misinterpreted as a single transport.

Regards,
Marton

>
> ---
>  libavformat/rtsp.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
> index b2b3f32011..f77de10119 100644
> --- a/libavformat/rtsp.c
> +++ b/libavformat/rtsp.c
> @@ -1001,11 +1001,9 @@ static void rtsp_parse_transport(AVFormatContext *s,
>
>              while (*p != ';' && *p != '\0' && *p != ',')
>                  p++;
> -            if (*p == ';')
> +            if (*p == ';' || *p == ',')
>                  p++;
>          }
> -        if (*p == ',')
> -            p++;
>
>          reply->nb_transports++;
>          if (reply->nb_transports >= RTSP_MAX_TRANSPORTS)
> --
> 2.21.1 (Apple Git-122.3)
> _______________________________________________
> 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".
diff mbox series

Patch

diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
index b2b3f32011..f77de10119 100644
--- a/libavformat/rtsp.c
+++ b/libavformat/rtsp.c
@@ -1001,11 +1001,9 @@  static void rtsp_parse_transport(AVFormatContext *s,

             while (*p != ';' && *p != '\0' && *p != ',')
                 p++;
-            if (*p == ';')
+            if (*p == ';' || *p == ',')
                 p++;
         }
-        if (*p == ',')
-            p++;

         reply->nb_transports++;
         if (reply->nb_transports >= RTSP_MAX_TRANSPORTS)