diff mbox series

[FFmpeg-devel] avformat/rtp: Pass sources and block filter addresses via sdp file for rtp

Message ID 20200406165651.58783-1-phunkyfish@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel] avformat/rtp: Pass sources and block filter addresses via sdp file for rtp | expand

Checks

Context Check Description
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Ross Nicholson April 6, 2020, 4:56 p.m. UTC 
---
 libavformat/rtsp.c | 47 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 39 insertions(+), 8 deletions(-)

Comments

Ross Nicholson April 6, 2020, 5:30 p.m. UTC | #1 
> sdp_length is used uninitialized here it is used uninitialized in the
> version that was merged as b71685865fe761925feedda3cd0b288224d9a509. The
> newer versions [2], [3] don't exhibit this flaw.
>
> [3] and [1] also have a flaw in common that [2] and this one are
> lacking: The semicolon of the definition of const char *p is missing.
>
> Finally, neither of these versions here seems to have been based upon
> git master which contains a call to av_log() directly after the above
> snprintf.
>
> - Andreas

Andreas,

Apologies I did not get back to you sooner I was indisposed.

I have updated the patch now which should be correct, addressing your
comments.

On Mon, 6 Apr 2020 at 17:56, phunkyfish <phunkyfish@gmail.com> wrote:

> ---
>  libavformat/rtsp.c | 47 ++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 39 insertions(+), 8 deletions(-)
>
> diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
> index cd6fc32a29..0d0bc2be0d 100644
> --- a/libavformat/rtsp.c
> +++ b/libavformat/rtsp.c
> @@ -2447,8 +2447,8 @@ static int rtp_probe(const AVProbeData *p)
>  static int rtp_read_header(AVFormatContext *s)
>  {
>      uint8_t recvbuf[RTP_MAX_PACKET_LENGTH];
> -    char host[500], sdp[500];
> -    int ret, port;
> +    char host[500], sdp[1000], filters_buf[1000];
> +    int ret, port, sdp_length, nc;
>      URLContext* in = NULL;
>      int payload_type;
>      AVCodecParameters *par = NULL;
> @@ -2456,6 +2456,7 @@ static int rtp_read_header(AVFormatContext *s)
>      AVIOContext pb;
>      socklen_t addrlen = sizeof(addr);
>      RTSPState *rt = s->priv_data;
> +    const char *p;
>
>      if (!ff_network_init())
>          return AVERROR(EIO);
> @@ -2513,13 +2514,40 @@ static int rtp_read_header(AVFormatContext *s)
>      av_url_split(NULL, 0, NULL, 0, host, sizeof(host), &port,
>                   NULL, 0, s->url);
>
> -    snprintf(sdp, sizeof(sdp),
> -             "v=0\r\nc=IN IP%d %s\r\nm=%s %d RTP/AVP %d\r\n",
> -             addr.ss_family == AF_INET ? 4 : 6, host,
> -             par->codec_type == AVMEDIA_TYPE_DATA  ? "application" :
> -             par->codec_type == AVMEDIA_TYPE_VIDEO ? "video" : "audio",
> -             port, payload_type);
> +    sdp_length = snprintf(sdp, sizeof(sdp),
> +                          "v=0\r\nc=IN IP%d %s\r\n",
> +                          addr.ss_family == AF_INET ? 4 : 6, host);
> +
> +    p = strchr(s->url, '?');
> +    if (p) {
> +        static const char *filters[][2] = {{"sources", "incl"}, {"block",
> "excl"}, {NULL, NULL}};
> +        int i;
> +        char *q;
> +        for (i = 0; filters[i][0]; i++) {
> +            if (av_find_info_tag(filters_buf, sizeof(filters_buf),
> filters[i][0], p)) {
> +                q = filters_buf;
> +                while ((q = strchr(q, ',')) != NULL)
> +                    *q = ' ';
> +                nc = snprintf(sdp + sdp_length, sizeof(sdp) - sdp_length,
> +                              "a=source-filter:%s IN IP%d %s %s\r\n",
> +                              filters[i][1],
> +                              addr.ss_family == AF_INET ? 4 : 6, host,
> +                              filters_buf);
> +                if (nc < 0 || nc + sdp_length >= sizeof(sdp))
> +                    goto fail_nobuf;
> +                sdp_length += nc;
> +            }
> +        }
> +    }
> +
> +    nc = snprintf(sdp + sdp_length, sizeof(sdp) - sdp_length,
> +                  "m=%s %d RTP/AVP %d\r\n",
> +                  par->codec_type == AVMEDIA_TYPE_DATA  ? "application" :
> +                  par->codec_type == AVMEDIA_TYPE_VIDEO ? "video" :
> "audio",
> +                  port, payload_type);
>      av_log(s, AV_LOG_VERBOSE, "SDP:\n%s\n", sdp);
> +    if (nc < 0 || nc + sdp_length >= sizeof(sdp))
> +        goto fail_nobuf;
>      avcodec_parameters_free(&par);
>
>      ffio_init_context(&pb, sdp, strlen(sdp), 0, NULL, NULL, NULL, NULL);
> @@ -2534,6 +2562,9 @@ static int rtp_read_header(AVFormatContext *s)
>      s->pb = NULL;
>      return ret;
>
> +fail_nobuf:
> +    ret = AVERROR(ENOBUFS);
> +    av_log(s, AV_LOG_ERROR, "rtp_read_header(): not enough buffer space
> for sdp-headers\n");
>  fail:
>      avcodec_parameters_free(&par);
>      if (in)
> --
> 2.24.1 (Apple Git-126)
>
>
Marton Balint April 6, 2020, 7:57 p.m. UTC | #2 
On Mon, 6 Apr 2020, phunkyfish wrote:

> ---
> libavformat/rtsp.c | 47 ++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 39 insertions(+), 8 deletions(-)
>
> diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
> index cd6fc32a29..0d0bc2be0d 100644
> --- a/libavformat/rtsp.c
> +++ b/libavformat/rtsp.c
> @@ -2447,8 +2447,8 @@ static int rtp_probe(const AVProbeData *p)
> static int rtp_read_header(AVFormatContext *s)
> {
>     uint8_t recvbuf[RTP_MAX_PACKET_LENGTH];
> -    char host[500], sdp[500];
> -    int ret, port;
> +    char host[500], sdp[1000], filters_buf[1000];
> +    int ret, port, sdp_length, nc;
>     URLContext* in = NULL;
>     int payload_type;
>     AVCodecParameters *par = NULL;
> @@ -2456,6 +2456,7 @@ static int rtp_read_header(AVFormatContext *s)
>     AVIOContext pb;
>     socklen_t addrlen = sizeof(addr);
>     RTSPState *rt = s->priv_data;
> +    const char *p;
>
>     if (!ff_network_init())
>         return AVERROR(EIO);
> @@ -2513,13 +2514,40 @@ static int rtp_read_header(AVFormatContext *s)
>     av_url_split(NULL, 0, NULL, 0, host, sizeof(host), &port,
>                  NULL, 0, s->url);
> 
> -    snprintf(sdp, sizeof(sdp),
> -             "v=0\r\nc=IN IP%d %s\r\nm=%s %d RTP/AVP %d\r\n",
> -             addr.ss_family == AF_INET ? 4 : 6, host,
> -             par->codec_type == AVMEDIA_TYPE_DATA  ? "application" :
> -             par->codec_type == AVMEDIA_TYPE_VIDEO ? "video" : "audio",
> -             port, payload_type);
> +    sdp_length = snprintf(sdp, sizeof(sdp),
> +                          "v=0\r\nc=IN IP%d %s\r\n",
> +                          addr.ss_family == AF_INET ? 4 : 6, host);

Please use an AVBPrint buffer instead of snprintf()-ing to sdp.

Thanks,
Marton

> +
> +    p = strchr(s->url, '?');
> +    if (p) {
> +        static const char *filters[][2] = {{"sources", "incl"}, {"block", "excl"}, {NULL, NULL}};
> +        int i;
> +        char *q;
> +        for (i = 0; filters[i][0]; i++) {
> +            if (av_find_info_tag(filters_buf, sizeof(filters_buf), filters[i][0], p)) {
> +                q = filters_buf;
> +                while ((q = strchr(q, ',')) != NULL)
> +                    *q = ' ';
> +                nc = snprintf(sdp + sdp_length, sizeof(sdp) - sdp_length,
> +                              "a=source-filter:%s IN IP%d %s %s\r\n",
> +                              filters[i][1],
> +                              addr.ss_family == AF_INET ? 4 : 6, host,
> +                              filters_buf);
> +                if (nc < 0 || nc + sdp_length >= sizeof(sdp))
> +                    goto fail_nobuf;
> +                sdp_length += nc;
> +            }
> +        }
> +    }
> +
> +    nc = snprintf(sdp + sdp_length, sizeof(sdp) - sdp_length,
> +                  "m=%s %d RTP/AVP %d\r\n",
> +                  par->codec_type == AVMEDIA_TYPE_DATA  ? "application" :
> +                  par->codec_type == AVMEDIA_TYPE_VIDEO ? "video" : "audio",
> +                  port, payload_type);
>     av_log(s, AV_LOG_VERBOSE, "SDP:\n%s\n", sdp);
> +    if (nc < 0 || nc + sdp_length >= sizeof(sdp))
> +        goto fail_nobuf;
>     avcodec_parameters_free(&par);
>
>     ffio_init_context(&pb, sdp, strlen(sdp), 0, NULL, NULL, NULL, NULL);
> @@ -2534,6 +2562,9 @@ static int rtp_read_header(AVFormatContext *s)
>     s->pb = NULL;
>     return ret;
> 
> +fail_nobuf:
> +    ret = AVERROR(ENOBUFS);
> +    av_log(s, AV_LOG_ERROR, "rtp_read_header(): not enough buffer space for sdp-headers\n");
> fail:
>     avcodec_parameters_free(&par);
>     if (in)
> -- 
> 2.24.1 (Apple Git-126)
>
> _______________________________________________
> 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".
Ross Nicholson April 6, 2020, 10:11 p.m. UTC | #3 
Ok, latest patch uses AVBPrint. Please let me know if my usage is correct,
I have not used it before.

In the meantime I will get some users to test this.

On Mon, 6 Apr 2020 at 20:57, Marton Balint <cus@passwd.hu> wrote:

>
>
> On Mon, 6 Apr 2020, phunkyfish wrote:
>
> > ---
> > libavformat/rtsp.c | 47 ++++++++++++++++++++++++++++++++++++++--------
> > 1 file changed, 39 insertions(+), 8 deletions(-)
> >
> > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
> > index cd6fc32a29..0d0bc2be0d 100644
> > --- a/libavformat/rtsp.c
> > +++ b/libavformat/rtsp.c
> > @@ -2447,8 +2447,8 @@ static int rtp_probe(const AVProbeData *p)
> > static int rtp_read_header(AVFormatContext *s)
> > {
> >     uint8_t recvbuf[RTP_MAX_PACKET_LENGTH];
> > -    char host[500], sdp[500];
> > -    int ret, port;
> > +    char host[500], sdp[1000], filters_buf[1000];
> > +    int ret, port, sdp_length, nc;
> >     URLContext* in = NULL;
> >     int payload_type;
> >     AVCodecParameters *par = NULL;
> > @@ -2456,6 +2456,7 @@ static int rtp_read_header(AVFormatContext *s)
> >     AVIOContext pb;
> >     socklen_t addrlen = sizeof(addr);
> >     RTSPState *rt = s->priv_data;
> > +    const char *p;
> >
> >     if (!ff_network_init())
> >         return AVERROR(EIO);
> > @@ -2513,13 +2514,40 @@ static int rtp_read_header(AVFormatContext *s)
> >     av_url_split(NULL, 0, NULL, 0, host, sizeof(host), &port,
> >                  NULL, 0, s->url);
> >
> > -    snprintf(sdp, sizeof(sdp),
> > -             "v=0\r\nc=IN IP%d %s\r\nm=%s %d RTP/AVP %d\r\n",
> > -             addr.ss_family == AF_INET ? 4 : 6, host,
> > -             par->codec_type == AVMEDIA_TYPE_DATA  ? "application" :
> > -             par->codec_type == AVMEDIA_TYPE_VIDEO ? "video" : "audio",
> > -             port, payload_type);
> > +    sdp_length = snprintf(sdp, sizeof(sdp),
> > +                          "v=0\r\nc=IN IP%d %s\r\n",
> > +                          addr.ss_family == AF_INET ? 4 : 6, host);
>
> Please use an AVBPrint buffer instead of snprintf()-ing to sdp.
>
> Thanks,
> Marton
>
> > +
> > +    p = strchr(s->url, '?');
> > +    if (p) {
> > +        static const char *filters[][2] = {{"sources", "incl"},
> {"block", "excl"}, {NULL, NULL}};
> > +        int i;
> > +        char *q;
> > +        for (i = 0; filters[i][0]; i++) {
> > +            if (av_find_info_tag(filters_buf, sizeof(filters_buf),
> filters[i][0], p)) {
> > +                q = filters_buf;
> > +                while ((q = strchr(q, ',')) != NULL)
> > +                    *q = ' ';
> > +                nc = snprintf(sdp + sdp_length, sizeof(sdp) -
> sdp_length,
> > +                              "a=source-filter:%s IN IP%d %s %s\r\n",
> > +                              filters[i][1],
> > +                              addr.ss_family == AF_INET ? 4 : 6, host,
> > +                              filters_buf);
> > +                if (nc < 0 || nc + sdp_length >= sizeof(sdp))
> > +                    goto fail_nobuf;
> > +                sdp_length += nc;
> > +            }
> > +        }
> > +    }
> > +
> > +    nc = snprintf(sdp + sdp_length, sizeof(sdp) - sdp_length,
> > +                  "m=%s %d RTP/AVP %d\r\n",
> > +                  par->codec_type == AVMEDIA_TYPE_DATA  ? "application"
> :
> > +                  par->codec_type == AVMEDIA_TYPE_VIDEO ? "video" :
> "audio",
> > +                  port, payload_type);
> >     av_log(s, AV_LOG_VERBOSE, "SDP:\n%s\n", sdp);
> > +    if (nc < 0 || nc + sdp_length >= sizeof(sdp))
> > +        goto fail_nobuf;
> >     avcodec_parameters_free(&par);
> >
> >     ffio_init_context(&pb, sdp, strlen(sdp), 0, NULL, NULL, NULL, NULL);
> > @@ -2534,6 +2562,9 @@ static int rtp_read_header(AVFormatContext *s)
> >     s->pb = NULL;
> >     return ret;
> >
> > +fail_nobuf:
> > +    ret = AVERROR(ENOBUFS);
> > +    av_log(s, AV_LOG_ERROR, "rtp_read_header(): not enough buffer space
> for sdp-headers\n");
> > fail:
> >     avcodec_parameters_free(&par);
> >     if (in)
> > --
> > 2.24.1 (Apple Git-126)
> >
> > _______________________________________________
> > 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".
> _______________________________________________
> 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 cd6fc32a29..0d0bc2be0d 100644
--- a/libavformat/rtsp.c
+++ b/libavformat/rtsp.c
@@ -2447,8 +2447,8 @@  static int rtp_probe(const AVProbeData *p)
 static int rtp_read_header(AVFormatContext *s)
 {
     uint8_t recvbuf[RTP_MAX_PACKET_LENGTH];
-    char host[500], sdp[500];
-    int ret, port;
+    char host[500], sdp[1000], filters_buf[1000];
+    int ret, port, sdp_length, nc;
     URLContext* in = NULL;
     int payload_type;
     AVCodecParameters *par = NULL;
@@ -2456,6 +2456,7 @@  static int rtp_read_header(AVFormatContext *s)
     AVIOContext pb;
     socklen_t addrlen = sizeof(addr);
     RTSPState *rt = s->priv_data;
+    const char *p;
 
     if (!ff_network_init())
         return AVERROR(EIO);
@@ -2513,13 +2514,40 @@  static int rtp_read_header(AVFormatContext *s)
     av_url_split(NULL, 0, NULL, 0, host, sizeof(host), &port,
                  NULL, 0, s->url);
 
-    snprintf(sdp, sizeof(sdp),
-             "v=0\r\nc=IN IP%d %s\r\nm=%s %d RTP/AVP %d\r\n",
-             addr.ss_family == AF_INET ? 4 : 6, host,
-             par->codec_type == AVMEDIA_TYPE_DATA  ? "application" :
-             par->codec_type == AVMEDIA_TYPE_VIDEO ? "video" : "audio",
-             port, payload_type);
+    sdp_length = snprintf(sdp, sizeof(sdp),
+                          "v=0\r\nc=IN IP%d %s\r\n",
+                          addr.ss_family == AF_INET ? 4 : 6, host);
+
+    p = strchr(s->url, '?');
+    if (p) {
+        static const char *filters[][2] = {{"sources", "incl"}, {"block", "excl"}, {NULL, NULL}};
+        int i;
+        char *q;
+        for (i = 0; filters[i][0]; i++) {
+            if (av_find_info_tag(filters_buf, sizeof(filters_buf), filters[i][0], p)) {
+                q = filters_buf;
+                while ((q = strchr(q, ',')) != NULL)
+                    *q = ' ';
+                nc = snprintf(sdp + sdp_length, sizeof(sdp) - sdp_length,
+                              "a=source-filter:%s IN IP%d %s %s\r\n",
+                              filters[i][1],
+                              addr.ss_family == AF_INET ? 4 : 6, host,
+                              filters_buf);
+                if (nc < 0 || nc + sdp_length >= sizeof(sdp))
+                    goto fail_nobuf;
+                sdp_length += nc;
+            }
+        }
+    }
+
+    nc = snprintf(sdp + sdp_length, sizeof(sdp) - sdp_length,
+                  "m=%s %d RTP/AVP %d\r\n",
+                  par->codec_type == AVMEDIA_TYPE_DATA  ? "application" :
+                  par->codec_type == AVMEDIA_TYPE_VIDEO ? "video" : "audio",
+                  port, payload_type);
     av_log(s, AV_LOG_VERBOSE, "SDP:\n%s\n", sdp);
+    if (nc < 0 || nc + sdp_length >= sizeof(sdp))
+        goto fail_nobuf;
     avcodec_parameters_free(&par);
 
     ffio_init_context(&pb, sdp, strlen(sdp), 0, NULL, NULL, NULL, NULL);
@@ -2534,6 +2562,9 @@  static int rtp_read_header(AVFormatContext *s)
     s->pb = NULL;
     return ret;
 
+fail_nobuf:
+    ret = AVERROR(ENOBUFS);
+    av_log(s, AV_LOG_ERROR, "rtp_read_header(): not enough buffer space for sdp-headers\n");
 fail:
     avcodec_parameters_free(&par);
     if (in)