diff mbox series

[FFmpeg-devel,v2] avformat/rtsp: load the sdp file with avio_read_to_bprint()

Message ID 1638420098-14559-1-git-send-email-lance.lmwang@gmail.com
State New
Headers show
Series [FFmpeg-devel,v2] avformat/rtsp: load the sdp file with avio_read_to_bprint()
Related show

Checks

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

Commit Message

Limin Wang Dec. 2, 2021, 4:41 a.m. UTC
From: Limin Wang <lance.lmwang@gmail.com>

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

Comments

Martin Storsjö Dec. 2, 2021, 8:04 a.m. UTC | #1
On Thu, 2 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 | 23 +++++++++--------------
> 1 file changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
> index e6a4993..249a019 100644
> --- a/libavformat/rtsp.c
> +++ b/libavformat/rtsp.c
> @@ -2373,9 +2373,10 @@ 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];
> +    AVBPrint bp;
>
>     if (!ff_network_init())
>         return AVERROR(EIO);
> @@ -2386,22 +2387,16 @@ 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);
> -    if (!content) {
> +    av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED);
> +    err = avio_read_to_bprint(s->pb, &bp, INT_MAX);
> +    if (err < 0 ) {
>         ff_network_close();
> -        return AVERROR(ENOMEM);
> +        av_bprint_finalize(&bp, NULL);
> +        return err;
>     }
> -    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';
> -
> +    content = bp.str;
>     err = ff_sdp_parse(s, content);

The content variable could be eliminated here too (to avoid leaving a 
dangling pointer local variable after freeing it).

> -    av_freep(&content);
> +    av_bprint_finalize(&bp, NULL);
>     if (err) goto fail;
>
>     /* open each RTP stream */
> -- 
> 1.8.3.1

This patch seems ok to me - maybe it'd be good to mention in the commit 
message that this allows getting rid of the hardcoded max size here, to 
explain the "why" for the patch, not only the "what"?

Would you be interested in getting rid of the same hardcoded max size in 
rtspdec.c too?

// Martin
Limin Wang Dec. 2, 2021, 10:22 a.m. UTC | #2
On Thu, Dec 02, 2021 at 10:04:37AM +0200, Martin Storsjö wrote:
> On Thu, 2 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 | 23 +++++++++--------------
> > 1 file changed, 9 insertions(+), 14 deletions(-)
> > 
> > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
> > index e6a4993..249a019 100644
> > --- a/libavformat/rtsp.c
> > +++ b/libavformat/rtsp.c
> > @@ -2373,9 +2373,10 @@ 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];
> > +    AVBPrint bp;
> > 
> >     if (!ff_network_init())
> >         return AVERROR(EIO);
> > @@ -2386,22 +2387,16 @@ 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);
> > -    if (!content) {
> > +    av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED);
> > +    err = avio_read_to_bprint(s->pb, &bp, INT_MAX);
> > +    if (err < 0 ) {
> >         ff_network_close();
> > -        return AVERROR(ENOMEM);
> > +        av_bprint_finalize(&bp, NULL);
> > +        return err;
> >     }
> > -    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';
> > -
> > +    content = bp.str;
> >     err = ff_sdp_parse(s, content);
> 
> The content variable could be eliminated here too (to avoid leaving a
> dangling pointer local variable after freeing it).

OK, will remove content.

> 
> > -    av_freep(&content);
> > +    av_bprint_finalize(&bp, NULL);
> >     if (err) goto fail;
> > 
> >     /* open each RTP stream */
> > -- 
> > 1.8.3.1
> 
> This patch seems ok to me - maybe it'd be good to mention in the commit
> message that this allows getting rid of the hardcoded max size here, to
> explain the "why" for the patch, not only the "what"?
OK, will add below comment message:
this allows getting rid of the hardcoded max size of SDP.

> 
> Would you be interested in getting rid of the same hardcoded max size in
> rtspdec.c too?

OK, will submit a patch for that.

> 
> // Martin
>
diff mbox series

Patch

diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
index e6a4993..249a019 100644
--- a/libavformat/rtsp.c
+++ b/libavformat/rtsp.c
@@ -2373,9 +2373,10 @@  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];
+    AVBPrint bp;
 
     if (!ff_network_init())
         return AVERROR(EIO);
@@ -2386,22 +2387,16 @@  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);
-    if (!content) {
+    av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED);
+    err = avio_read_to_bprint(s->pb, &bp, INT_MAX);
+    if (err < 0 ) {
         ff_network_close();
-        return AVERROR(ENOMEM);
+        av_bprint_finalize(&bp, NULL);
+        return err;
     }
-    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';
-
+    content = bp.str;
     err = ff_sdp_parse(s, content);
-    av_freep(&content);
+    av_bprint_finalize(&bp, NULL);
     if (err) goto fail;
 
     /* open each RTP stream */