diff mbox

[FFmpeg-devel] avformat/rtsp: check return value of read in ff_rtsp_read_reply()

Message ID 1494256907.592817.59A342FE@webmail.messagingengine.com
State New
Headers show

Commit Message

Daniel Richard G. May 8, 2017, 3:24 p.m. UTC 
Resending. Are there any objections to this patch?

On Mon, 2017 Apr 17 01:12-0400, Daniel Richard G. wrote:
> In the course of testing RTSP streaming of CCTV video via the FFmpeg
> API, I have found some Valgrind uninitialized-memory errors due to what
> appear to be short/failed reads in ffurl_read_complete().
> 
> The calling function ff_rtsp_read_reply() was not checking the return
> value, and so the library went on to parse garbage in an
> uninitialized heap-allocated buffer.
> 
> The attached patch adds logic to check the return value and bail
> out on error.
> 
> 
> --Daniel
>

Comments

wm4 May 8, 2017, 4:04 p.m. UTC | #1 
On Mon, 08 May 2017 11:24:19 -0400
"Daniel Richard G." <skunk@iSKUNK.ORG> wrote:

> From 477cbd18b630365d612da173201c2e4ee763d7d4 Mon Sep 17 00:00:00 2001
> From: Daniel Richard G <skunk@iSKUNK.ORG>
> Date: Sun, 16 Apr 2017 23:12:53 -0400
> Subject: [PATCH] avformat/rtsp: check return value of read in ff_rtsp_read_reply()
> 
> Signed-off-by: Daniel Richard G <skunk@iSKUNK.ORG>
> ---
>  libavformat/rtsp.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
> index 261e970..da962fb 100644
> --- a/libavformat/rtsp.c
> +++ b/libavformat/rtsp.c
> @@ -1218,7 +1218,11 @@ start:
>          content = av_malloc(content_length + 1);
>          if (!content)
>              return AVERROR(ENOMEM);
> -        ffurl_read_complete(rt->rtsp_hd, content, content_length);
> +        ret = ffurl_read_complete(rt->rtsp_hd, content, content_length);
> +        if (ret != content_length) {
> +            av_freep(&content);
> +            return AVERROR_EOF;
> +        }
>          content[content_length] = '\0';
>      }
>      if (content_ptr)

That's probably better than before, so LGTM.

Can ffurl_read_complete() return an error value as well? If so, it
might be better to return the error value when that happens.
Daniel Richard G. May 8, 2017, 8:48 p.m. UTC | #2 
On Mon, 2017 May  8 18:04+0200, wm4 wrote:
> On Mon, 08 May 2017 11:24:19 -0400
> "Daniel Richard G." <skunk@iSKUNK.ORG> wrote:
> 
> > From 477cbd18b630365d612da173201c2e4ee763d7d4 Mon Sep 17 00:00:00 2001
> > From: Daniel Richard G <skunk@iSKUNK.ORG>
> > Date: Sun, 16 Apr 2017 23:12:53 -0400
> > Subject: [PATCH] avformat/rtsp: check return value of read in ff_rtsp_read_reply()
> > 
> > Signed-off-by: Daniel Richard G <skunk@iSKUNK.ORG>
> > ---
> >  libavformat/rtsp.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
> > index 261e970..da962fb 100644
> > --- a/libavformat/rtsp.c
> > +++ b/libavformat/rtsp.c
> > @@ -1218,7 +1218,11 @@ start:
> >          content = av_malloc(content_length + 1);
> >          if (!content)
> >              return AVERROR(ENOMEM);
> > -        ffurl_read_complete(rt->rtsp_hd, content, content_length);
> > +        ret = ffurl_read_complete(rt->rtsp_hd, content, content_length);
> > +        if (ret != content_length) {
> > +            av_freep(&content);
> > +            return AVERROR_EOF;
> > +        }
> >          content[content_length] = '\0';
> >      }
> >      if (content_ptr)
> 
> That's probably better than before, so LGTM.
> 
> Can ffurl_read_complete() return an error value as well? If so, it
> might be better to return the error value when that happens.

Agreed; my point of reference was the 1-byte read that occurs a few
lines up, under the "parse reply (XXX: use buffers)" comment. The
calling function already assumes AVERROR_EOF given a short read.


--Daniel
diff mbox

Patch

From 477cbd18b630365d612da173201c2e4ee763d7d4 Mon Sep 17 00:00:00 2001
From: Daniel Richard G <skunk@iSKUNK.ORG>
Date: Sun, 16 Apr 2017 23:12:53 -0400
Subject: [PATCH] avformat/rtsp: check return value of read in ff_rtsp_read_reply()

Signed-off-by: Daniel Richard G <skunk@iSKUNK.ORG>
---
 libavformat/rtsp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
index 261e970..da962fb 100644
--- a/libavformat/rtsp.c
+++ b/libavformat/rtsp.c
@@ -1218,7 +1218,11 @@  start:
         content = av_malloc(content_length + 1);
         if (!content)
             return AVERROR(ENOMEM);
-        ffurl_read_complete(rt->rtsp_hd, content, content_length);
+        ret = ffurl_read_complete(rt->rtsp_hd, content, content_length);
+        if (ret != content_length) {
+            av_freep(&content);
+            return AVERROR_EOF;
+        }
         content[content_length] = '\0';
     }
     if (content_ptr)
-- 
2.9.0