[FFmpeg-devel] [PATCH] ffprobe: fix use of uninitialized variable

[Michael Niedermayer]

On Wed, Aug 09, 2017 at 01:05:11PM +0800, Zhao Zhili wrote:
> On Wed, Aug 9, 2017 at 1:23 AM, Michael Niedermayer <michael at niedermayer.cc>
> wrote:
> 
> > On Sun, Aug 06, 2017 at 03:39:07PM +0800, Zhao Zhili wrote:
> > > ---
> > >  ffprobe.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/ffprobe.c b/ffprobe.c
> > > index f22c4f57ad..ac9ff051fa 100644
> > > --- a/ffprobe.c
> > > +++ b/ffprobe.c
> > > @@ -3328,6 +3328,7 @@ static int parse_read_interval(const char
> > > *interval_spec,
> > >              }
> > >              interval->end = lli;
> > >          } else {
> > > +            interval->duration_frames = 0;
> > >              ret = av_parse_time(&us, p, 1);
> > >              if (ret < 0) {
> > >                  av_log(NULL, AV_LOG_ERROR, "Invalid interval
> > end/duration
> > > specification '%s'\n", p);
> > > @@ -3337,6 +3338,7 @@ static int parse_read_interval(const char
> > > *interval_spec,
> > >          }
> > >      } else {
> > >          interval->has_end = 0;
> > > +        interval->duration_frames = 0;
> > >      }
> > >
> > >  end:
> > > @@ -3357,7 +3359,7 @@ static int parse_read_intervals(const char
> > > *intervals_spec)
> > >              n++;
> > >      n++;
> > >
> > > -    read_intervals = av_malloc_array(n, sizeof(*read_intervals));
> > > +    read_intervals = av_mallocz_array(n, sizeof(*read_intervals));
> > >      if (!read_intervals) {
> > >          ret = AVERROR(ENOMEM);
> > >          goto end;
> > > --
> > > 2.13.2
> >
> > >  ffprobe.c |    4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 59efe1bf808d9621c14252fd0e71dc4b9a635c7a  0001-ffprobe-fix-use-of-
> > uninitialized-variable.patch
> > > From 1d3fe1eb4cd27efc337a1fae302f5539e3c7459b Mon Sep 17 00:00:00 2001
> > > From: Zhao Zhili <wantlamy at gmail.com>
> > > Date: Sun, 6 Aug 2017 15:33:42 +0800
> > > Subject: [PATCH] ffprobe: fix use of uninitialized variable
> > >
> > > ---
> > >  ffprobe.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/ffprobe.c b/ffprobe.c
> > > index f22c4f57ad..ac9ff051fa 100644
> > > --- a/ffprobe.c
> > > +++ b/ffprobe.c
> > > @@ -3328,6 +3328,7 @@ static int parse_read_interval(const char
> > *interval_spec,
> > >              }
> > >              interval->end = lli;
> > >          } else {
> > > +            interval->duration_frames = 0;
> > >              ret = av_parse_time(&us, p, 1);
> > >              if (ret < 0) {
> > >                  av_log(NULL, AV_LOG_ERROR, "Invalid interval
> > end/duration specification '%s'\n", p);
> >

> > > @@ -3337,6 +3338,7 @@ static int parse_read_interval(const char
> > *interval_spec,
> > >          }
> > >      } else {
> > >          interval->has_end = 0;
> > > +        interval->duration_frames = 0;
> > >      }
> >
> > isnt has_end and end_is_offset 0 here and all uses of duration_frames
> > under code checking either ?
> >
> >
> If end_is_offset is 1 and *p is not '#', then duration_frames is not
> initialized.

The code quoted above is under the else, while the only code setting
end_is_offset is under the if() side.
how can you have end_is_offset == 1 without duration_frames being
set ?

further your patch allocates the intervals with av_mallocz_array()
how can they be uninitialized if they are cleared during allocation?

To me it looks like your code initializes the variable 3 times
2 are unneeded.
But this is not code i know well so it may be that iam missing
something



> So check end_is_offset doesn't prevent access uninitialized
> value. And the current code didn't always check end_is_offset before access
> duration_frames.
> 
> A simple test case:
> ffprobe -show_frames -select_streams v -read_intervals '%+60' foo

yes i can drop 2 hunks of the patch and it still fixes the issue

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who would give up essential Liberty, to purchase a little
temporary Safety, deserve neither Liberty nor Safety -- Benjamin Franklin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170816/00215f9c/attachment.sig>