diff mbox series

[FFmpeg-devel,v2,4/6] avformat/rtsp: check content_ptr before memory allocate

Message ID 1638765904-2521-4-git-send-email-lance.lmwang@gmail.com
State New
Headers show
Series [FFmpeg-devel,v2,1/6] avformat/rtsp: remove redundant assignment | 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. 6, 2021, 4:45 a.m. UTC
From: Limin Wang <lance.lmwang@gmail.com>

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

Comments

Martin Storsjö Dec. 7, 2021, 9:30 a.m. UTC | #1
On Mon, 6 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 | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
> index 3e4a68a..b3d1e91 100644
> --- a/libavformat/rtsp.c
> +++ b/libavformat/rtsp.c
> @@ -1240,7 +1240,7 @@ start:
>         av_strlcpy(rt->session_id, reply->session_id, sizeof(rt->session_id));
>
>     content_length = reply->content_length;
> -    if (content_length > 0) {
> +    if (content_ptr && content_length > 0) {
>         /* leave some room for a trailing '\0' (useful for simple parsing) */
>         content = av_malloc(content_length + 1);
>         if (!content)
> @@ -1250,11 +1250,8 @@ start:
>             return AVERROR(EIO);
>         }
>         content[content_length] = '\0';
> -    }
> -    if (content_ptr)
>         *content_ptr = content;
> -    else
> -        av_freep(&content);
> +    }

I don't think this is right.

If the reply that we read contain a body of contents, but the caller 
didn't pass any content_ptr, then we still need to read and consume the 
body of contents from the stream, even if we don't pass it to the caller. 
(Maybe we should warn in this case, that there was some potentially 
relevant data that the caller didn't care about?) But if we don't read it, 
like this patch does, we would end up desynced from the protocol stream.

// Martin
Limin Wang Dec. 7, 2021, 10:25 a.m. UTC | #2
On Tue, Dec 07, 2021 at 11:30:11AM +0200, Martin Storsjö wrote:
> On Mon, 6 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 | 7 ++-----
> > 1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
> > index 3e4a68a..b3d1e91 100644
> > --- a/libavformat/rtsp.c
> > +++ b/libavformat/rtsp.c
> > @@ -1240,7 +1240,7 @@ start:
> >         av_strlcpy(rt->session_id, reply->session_id, sizeof(rt->session_id));
> > 
> >     content_length = reply->content_length;
> > -    if (content_length > 0) {
> > +    if (content_ptr && content_length > 0) {
> >         /* leave some room for a trailing '\0' (useful for simple parsing) */
> >         content = av_malloc(content_length + 1);
> >         if (!content)
> > @@ -1250,11 +1250,8 @@ start:
> >             return AVERROR(EIO);
> >         }
> >         content[content_length] = '\0';
> > -    }
> > -    if (content_ptr)
> >         *content_ptr = content;
> > -    else
> > -        av_freep(&content);
> > +    }
> 
> I don't think this is right.
> 
> If the reply that we read contain a body of contents, but the caller didn't
> pass any content_ptr, then we still need to read and consume the body of
> contents from the stream, even if we don't pass it to the caller. (Maybe we
> should warn in this case, that there was some potentially relevant data that
> the caller didn't care about?) But if we don't read it, like this patch
> does, we would end up desynced from the protocol stream.

Yes, I haven't realized your valid case, ignore this change anyway. 

> 
> // Martin
>
diff mbox series

Patch

diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
index 3e4a68a..b3d1e91 100644
--- a/libavformat/rtsp.c
+++ b/libavformat/rtsp.c
@@ -1240,7 +1240,7 @@  start:
         av_strlcpy(rt->session_id, reply->session_id, sizeof(rt->session_id));
 
     content_length = reply->content_length;
-    if (content_length > 0) {
+    if (content_ptr && content_length > 0) {
         /* leave some room for a trailing '\0' (useful for simple parsing) */
         content = av_malloc(content_length + 1);
         if (!content)
@@ -1250,11 +1250,8 @@  start:
             return AVERROR(EIO);
         }
         content[content_length] = '\0';
-    }
-    if (content_ptr)
         *content_ptr = content;
-    else
-        av_freep(&content);
+    }
 
     if (request) {
         char buf[MAX_URL_SIZE];