diff mbox series

[FFmpeg-devel,1/5] avformat/rtsp: make use of avio_get_str() to load the sdp

Message ID 1638365954-29420-1-git-send-email-lance.lmwang@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/5] avformat/rtsp: make use of avio_get_str() to load the sdp | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Limin Wang Dec. 1, 2021, 1:39 p.m. UTC
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavformat/rtsp.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

Comments

Martin Storsjö Dec. 1, 2021, 1:55 p.m. UTC | #1
On Wed, 1 Dec 2021, lance.lmwang@gmail.com wrote:

> From: Limin Wang <lance.lmwang@gmail.com>
>
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
> libavformat/rtsp.c | 30 ++++++++++++++++++++----------
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
> index e6a4993..ec8be8b 100644
> --- a/libavformat/rtsp.c
> +++ b/libavformat/rtsp.c
> @@ -2369,11 +2369,29 @@ static void append_source_addrs(char *buf, int size, const char *name,
>         av_strlcatf(buf, size, ",%s", addrs[i]->addr);
> }
>
> +static char *read_sdp_str(AVIOContext *pb, int size)
> +{
> +    int n;
> +    char *str;
> +
> +    if (size < 0 || size == INT_MAX)
> +        return NULL;
> +
> +    str = av_malloc(size + 1);
> +    if (!str)
> +        return NULL;
> +    n = avio_get_str(pb, size, str, size + 1);
> +    if (n < size)
> +        avio_skip(pb, size - n);

What? Why do you want to skip past data in the input? Size here is just a 
random static upper limit of how large an SDP file would be.

This patch totally lacks a motivation of why you want to do this.

// Martin
Limin Wang Dec. 1, 2021, 2:31 p.m. UTC | #2
On Wed, Dec 01, 2021 at 03:55:37PM +0200, Martin Storsjö wrote:
> On Wed, 1 Dec 2021, lance.lmwang@gmail.com wrote:
> 
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> > libavformat/rtsp.c | 30 ++++++++++++++++++++----------
> > 1 file changed, 20 insertions(+), 10 deletions(-)
> > 
> > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
> > index e6a4993..ec8be8b 100644
> > --- a/libavformat/rtsp.c
> > +++ b/libavformat/rtsp.c
> > @@ -2369,11 +2369,29 @@ static void append_source_addrs(char *buf, int size, const char *name,
> >         av_strlcatf(buf, size, ",%s", addrs[i]->addr);
> > }
> > 
> > +static char *read_sdp_str(AVIOContext *pb, int size)
> > +{
> > +    int n;
> > +    char *str;
> > +
> > +    if (size < 0 || size == INT_MAX)
> > +        return NULL;
> > +
> > +    str = av_malloc(size + 1);
> > +    if (!str)
> > +        return NULL;
> > +    n = avio_get_str(pb, size, str, size + 1);
> > +    if (n < size)
> > +        avio_skip(pb, size - n);
> 
> What? Why do you want to skip past data in the input? Size here is just a
> random static upper limit of how large an SDP file would be.
yes, my fault, don't need to run avio_skip(). For avio_get_str() will return error only
buflen <=0, so str is valid anyway.

> 
> This patch totally lacks a motivation of why you want to do this.

considering the SDP is strings in file, so I think it's better to use avio_get_str().
Also, I'm considering to remove limit of SDP_MAX_SIZE after that.

> 
> // Martin
>
Martin Storsjö Dec. 1, 2021, 3:17 p.m. UTC | #3
On Wed, 1 Dec 2021, lance.lmwang@gmail.com wrote:

> On Wed, Dec 01, 2021 at 03:55:37PM +0200, Martin Storsjö wrote:
>> On Wed, 1 Dec 2021, lance.lmwang@gmail.com wrote:
>> 
>> > From: Limin Wang <lance.lmwang@gmail.com>
>> > 
>> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>> > ---
>> > libavformat/rtsp.c | 30 ++++++++++++++++++++----------
>> > 1 file changed, 20 insertions(+), 10 deletions(-)
>> > 
>> > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
>> > index e6a4993..ec8be8b 100644
>> > --- a/libavformat/rtsp.c
>> > +++ b/libavformat/rtsp.c
>> > @@ -2369,11 +2369,29 @@ static void append_source_addrs(char *buf, int size, const char *name,
>> >         av_strlcatf(buf, size, ",%s", addrs[i]->addr);
>> > }
>> > 
>> > +static char *read_sdp_str(AVIOContext *pb, int size)
>> > +{
>> > +    int n;
>> > +    char *str;
>> > +
>> > +    if (size < 0 || size == INT_MAX)
>> > +        return NULL;
>> > +
>> > +    str = av_malloc(size + 1);
>> > +    if (!str)
>> > +        return NULL;
>> > +    n = avio_get_str(pb, size, str, size + 1);
>> > +    if (n < size)
>> > +        avio_skip(pb, size - n);
>> 
>> What? Why do you want to skip past data in the input? Size here is just a
>> random static upper limit of how large an SDP file would be.
> yes, my fault, don't need to run avio_skip(). For avio_get_str() will return error only
> buflen <=0, so str is valid anyway.
>
>> 
>> This patch totally lacks a motivation of why you want to do this.
>
> considering the SDP is strings in file, so I think it's better to use avio_get_str().

Maybe, but warranting a patch changing things on its own, IMO.

> Also, I'm considering to remove limit of SDP_MAX_SIZE after that.

If you'd defer this patch to a patchset that does that, it might make more 
sense. But I don't see much value in applying it on its own.

// Martin
Limin Wang Dec. 1, 2021, 3:32 p.m. UTC | #4
On Wed, Dec 01, 2021 at 05:17:08PM +0200, Martin Storsjö wrote:
> On Wed, 1 Dec 2021, lance.lmwang@gmail.com wrote:
> 
> > On Wed, Dec 01, 2021 at 03:55:37PM +0200, Martin Storsjö wrote:
> > > On Wed, 1 Dec 2021, lance.lmwang@gmail.com wrote:
> > > 
> > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > > ---
> > > > libavformat/rtsp.c | 30 ++++++++++++++++++++----------
> > > > 1 file changed, 20 insertions(+), 10 deletions(-)
> > > > > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
> > > > index e6a4993..ec8be8b 100644
> > > > --- a/libavformat/rtsp.c
> > > > +++ b/libavformat/rtsp.c
> > > > @@ -2369,11 +2369,29 @@ static void append_source_addrs(char *buf, int size, const char *name,
> > > >         av_strlcatf(buf, size, ",%s", addrs[i]->addr);
> > > > }
> > > > > +static char *read_sdp_str(AVIOContext *pb, int size)
> > > > +{
> > > > +    int n;
> > > > +    char *str;
> > > > +
> > > > +    if (size < 0 || size == INT_MAX)
> > > > +        return NULL;
> > > > +
> > > > +    str = av_malloc(size + 1);
> > > > +    if (!str)
> > > > +        return NULL;
> > > > +    n = avio_get_str(pb, size, str, size + 1);
> > > > +    if (n < size)
> > > > +        avio_skip(pb, size - n);
> > > 
> > > What? Why do you want to skip past data in the input? Size here is just a
> > > random static upper limit of how large an SDP file would be.
> > yes, my fault, don't need to run avio_skip(). For avio_get_str() will return error only
> > buflen <=0, so str is valid anyway.
> > 
> > > 
> > > This patch totally lacks a motivation of why you want to do this.
> > 
> > considering the SDP is strings in file, so I think it's better to use avio_get_str().
> 
> Maybe, but warranting a patch changing things on its own, IMO.
> 
> > Also, I'm considering to remove limit of SDP_MAX_SIZE after that.
> 
> If you'd defer this patch to a patchset that does that, it might make more
> sense. But I don't see much value in applying it on its own.

Yes, I consider to add new api which is avio_get_str_len() to get the string len from pb, 
before that, I want to hear no objection to use avio_get_str(). then I'll submit a patchset 
for the complete change for review.

> 
> // Martin
> _______________________________________________
> 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".
Marton Balint Dec. 1, 2021, 5:55 p.m. UTC | #5
On Wed, 1 Dec 2021, lance.lmwang@gmail.com wrote:

> On Wed, Dec 01, 2021 at 05:17:08PM +0200, Martin Storsjö wrote:
>> On Wed, 1 Dec 2021, lance.lmwang@gmail.com wrote:
>> 
>> > On Wed, Dec 01, 2021 at 03:55:37PM +0200, Martin Storsjö wrote:
>> > > On Wed, 1 Dec 2021, lance.lmwang@gmail.com wrote:
>> > > 
>> > > > From: Limin Wang <lance.lmwang@gmail.com>
>> > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>> > > > ---
>> > > > libavformat/rtsp.c | 30 ++++++++++++++++++++----------
>> > > > 1 file changed, 20 insertions(+), 10 deletions(-)
>> > > > > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
>> > > > index e6a4993..ec8be8b 100644
>> > > > --- a/libavformat/rtsp.c
>> > > > +++ b/libavformat/rtsp.c
>> > > > @@ -2369,11 +2369,29 @@ static void append_source_addrs(char *buf, int size, const char *name,
>> > > >         av_strlcatf(buf, size, ",%s", addrs[i]->addr);
>> > > > }
>> > > > > +static char *read_sdp_str(AVIOContext *pb, int size)
>> > > > +{
>> > > > +    int n;
>> > > > +    char *str;
>> > > > +
>> > > > +    if (size < 0 || size == INT_MAX)
>> > > > +        return NULL;
>> > > > +
>> > > > +    str = av_malloc(size + 1);
>> > > > +    if (!str)
>> > > > +        return NULL;
>> > > > +    n = avio_get_str(pb, size, str, size + 1);
>> > > > +    if (n < size)
>> > > > +        avio_skip(pb, size - n);
>> > > 
>> > > What? Why do you want to skip past data in the input? Size here is just a
>> > > random static upper limit of how large an SDP file would be.
>> > yes, my fault, don't need to run avio_skip(). For avio_get_str() will return error only
>> > buflen <=0, so str is valid anyway.
>> > 
>> > > 
>> > > This patch totally lacks a motivation of why you want to do this.
>> > 
>> > considering the SDP is strings in file, so I think it's better to use avio_get_str().
>> 
>> Maybe, but warranting a patch changing things on its own, IMO.
>> 
>> > Also, I'm considering to remove limit of SDP_MAX_SIZE after that.
>> 
>> If you'd defer this patch to a patchset that does that, it might make more
>> sense. But I don't see much value in applying it on its own.
>
> Yes, I consider to add new api which is avio_get_str_len() to get the string len from pb, 
> before that, I want to hear no objection to use avio_get_str(). then I'll submit a patchset 
> for the complete change for review.

I thought you simply want to read the file to memory. You don't need new 
API function to do that, simply use avio_read_to_bprint() with INT_MAX 
upper limit. It will allocate the necessary buffer for you, and you can 
check if ENOMEM happened with av_bprint_is_complete(), and free the buffer 
after parsing with av_bprint_finalize().

Regards,
Marton
Limin Wang Dec. 2, 2021, 1:04 a.m. UTC | #6
On Wed, Dec 01, 2021 at 06:55:19PM +0100, Marton Balint wrote:
> 
> 
> On Wed, 1 Dec 2021, lance.lmwang@gmail.com wrote:
> 
> > On Wed, Dec 01, 2021 at 05:17:08PM +0200, Martin Storsjö wrote:
> > > On Wed, 1 Dec 2021, lance.lmwang@gmail.com wrote:
> > > 
> > > > On Wed, Dec 01, 2021 at 03:55:37PM +0200, Martin Storsjö wrote:
> > > > > On Wed, 1 Dec 2021, lance.lmwang@gmail.com wrote:
> > > > > > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > > > > ---
> > > > > > libavformat/rtsp.c | 30 ++++++++++++++++++++----------
> > > > > > 1 file changed, 20 insertions(+), 10 deletions(-)
> > > > > > > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
> > > > > > index e6a4993..ec8be8b 100644
> > > > > > --- a/libavformat/rtsp.c
> > > > > > +++ b/libavformat/rtsp.c
> > > > > > @@ -2369,11 +2369,29 @@ static void append_source_addrs(char *buf, int size, const char *name,
> > > > > >         av_strlcatf(buf, size, ",%s", addrs[i]->addr);
> > > > > > }
> > > > > > > +static char *read_sdp_str(AVIOContext *pb, int size)
> > > > > > +{
> > > > > > +    int n;
> > > > > > +    char *str;
> > > > > > +
> > > > > > +    if (size < 0 || size == INT_MAX)
> > > > > > +        return NULL;
> > > > > > +
> > > > > > +    str = av_malloc(size + 1);
> > > > > > +    if (!str)
> > > > > > +        return NULL;
> > > > > > +    n = avio_get_str(pb, size, str, size + 1);
> > > > > > +    if (n < size)
> > > > > > +        avio_skip(pb, size - n);
> > > > > > > What? Why do you want to skip past data in the input? Size
> > > here is just a
> > > > > random static upper limit of how large an SDP file would be.
> > > > yes, my fault, don't need to run avio_skip(). For avio_get_str() will return error only
> > > > buflen <=0, so str is valid anyway.
> > > > > > > > This patch totally lacks a motivation of why you want to
> > > do this.
> > > > > considering the SDP is strings in file, so I think it's better
> > > to use avio_get_str().
> > > 
> > > Maybe, but warranting a patch changing things on its own, IMO.
> > > 
> > > > Also, I'm considering to remove limit of SDP_MAX_SIZE after that.
> > > 
> > > If you'd defer this patch to a patchset that does that, it might make more
> > > sense. But I don't see much value in applying it on its own.
> > 
> > Yes, I consider to add new api which is avio_get_str_len() to get the
> > string len from pb, before that, I want to hear no objection to use
> > avio_get_str(). then I'll submit a patchset for the complete change for
> > review.
> 
> I thought you simply want to read the file to memory. You don't need new API
> function to do that, simply use avio_read_to_bprint() with INT_MAX upper
> limit. It will allocate the necessary buffer for you, and you can check if
> ENOMEM happened with av_bprint_is_complete(), and free the buffer after
> parsing with av_bprint_finalize().

that's cool, I haven't notice avio_read_to_bprint() existing, will try 
it.


> 
> Regards,
> Marton
> _______________________________________________
> 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 e6a4993..ec8be8b 100644
--- a/libavformat/rtsp.c
+++ b/libavformat/rtsp.c
@@ -2369,11 +2369,29 @@  static void append_source_addrs(char *buf, int size, const char *name,
         av_strlcatf(buf, size, ",%s", addrs[i]->addr);
 }
 
+static char *read_sdp_str(AVIOContext *pb, int size)
+{
+    int n;
+    char *str;
+
+    if (size < 0 || size == INT_MAX)
+        return NULL;
+
+    str = av_malloc(size + 1);
+    if (!str)
+        return NULL;
+    n = avio_get_str(pb, size, str, size + 1);
+    if (n < size)
+        avio_skip(pb, size - n);
+    return str;
+}
+
+
 static int sdp_read_header(AVFormatContext *s)
 {
     RTSPState *rt = s->priv_data;
     RTSPStream *rtsp_st;
-    int size, i, err;
+    int i, err;
     char *content;
     char url[MAX_URL_SIZE];
 
@@ -2386,19 +2404,11 @@  static int sdp_read_header(AVFormatContext *s)
         rt->lower_transport = RTSP_LOWER_TRANSPORT_CUSTOM;
 
     /* read the whole sdp file */
-    /* XXX: better loading */
-    content = av_malloc(SDP_MAX_SIZE);
+    content = read_sdp_str(s->pb, SDP_MAX_SIZE - 1);
     if (!content) {
         ff_network_close();
         return AVERROR(ENOMEM);
     }
-    size = avio_read(s->pb, content, SDP_MAX_SIZE - 1);
-    if (size <= 0) {
-        av_free(content);
-        ff_network_close();
-        return AVERROR_INVALIDDATA;
-    }
-    content[size] ='\0';
 
     err = ff_sdp_parse(s, content);
     av_freep(&content);