[FFmpeg-devel,v2] ffmpeg: fix uninitialized return value

Submitted by Tobias Rapp on Nov. 30, 2016, 2:26 p.m.

Details

Message ID a742462e-51a3-f7ec-7fb6-b84c023d09a2@noa-archive.com
State New
Headers show

Commit Message

Tobias Rapp Nov. 30, 2016, 2:26 p.m.
On 22.11.2016 15:34, Tobias Rapp wrote:
> On 22.11.2016 15:06, Michael Niedermayer wrote:
>> On Tue, Nov 22, 2016 at 02:43:57PM +0100, Tobias Rapp wrote:
>>> On 22.11.2016 14:34, Michael Niedermayer wrote:
>>>> On Tue, Nov 22, 2016 at 09:16:26AM +0100, Tobias Rapp wrote:
>>>>> Signed-off-by: Tobias Rapp <t.rapp@noa-archive.com>
>>>>> ---
>>>>> ffmpeg.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> do you have a testcase for this ?
>>>
>>> No, I just stumbled over it when reading the code.
>>
>> are you sure that the codepath leaving ret uninitialized is possible ?
>>
>> if not adding a av_assert() may be better
>
> From my quick glance I assumed that ist->nb_filters could be zero and
> the for-loop is not entered. If that should never be the case I agree
> that an added av_assert(ist->nb_filters > 0) would be better.

I added an assert and it is not triggered when running FATE, see 
attached patch.

Regards,
Tobias

Comments

Michael Niedermayer Dec. 1, 2016, 11:01 a.m.
On Wed, Nov 30, 2016 at 03:26:04PM +0100, Tobias Rapp wrote:
> On 22.11.2016 15:34, Tobias Rapp wrote:
> >On 22.11.2016 15:06, Michael Niedermayer wrote:
> >>On Tue, Nov 22, 2016 at 02:43:57PM +0100, Tobias Rapp wrote:
> >>>On 22.11.2016 14:34, Michael Niedermayer wrote:
> >>>>On Tue, Nov 22, 2016 at 09:16:26AM +0100, Tobias Rapp wrote:
> >>>>>Signed-off-by: Tobias Rapp <t.rapp@noa-archive.com>
> >>>>>---
> >>>>>ffmpeg.c | 2 +-
> >>>>>1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>>do you have a testcase for this ?
> >>>
> >>>No, I just stumbled over it when reading the code.
> >>
> >>are you sure that the codepath leaving ret uninitialized is possible ?
> >>
> >>if not adding a av_assert() may be better
> >
> >From my quick glance I assumed that ist->nb_filters could be zero and
> >the for-loop is not entered. If that should never be the case I agree
> >that an added av_assert(ist->nb_filters > 0) would be better.
> 
> I added an assert and it is not triggered when running FATE, see
> attached patch.
> 
> Regards,
> Tobias

>  ffmpeg.c |    1 +
>  1 file changed, 1 insertion(+)
> a88ad0978feedd41c833a943b21cb6e954b663e4  0001-ffmpeg-assert-return-value-is-initialized.patch
> From a270cfdb637a48aca12d492cf4cb72d9200b6024 Mon Sep 17 00:00:00 2001
> From: Tobias Rapp <t.rapp@noa-archive.com>
> Date: Thu, 24 Nov 2016 15:45:00 +0100
> Subject: [PATCH 1/2] ffmpeg: assert return value is initialized
> 
> Signed-off-by: Tobias Rapp <t.rapp@noa-archive.com>
> ---
>  ffmpeg.c | 1 +
>  1 file changed, 1 insertion(+)

applied

thx

[...]

Patch hide | download patch | download mbox

From a270cfdb637a48aca12d492cf4cb72d9200b6024 Mon Sep 17 00:00:00 2001
From: Tobias Rapp <t.rapp@noa-archive.com>
Date: Thu, 24 Nov 2016 15:45:00 +0100
Subject: [PATCH 1/2] ffmpeg: assert return value is initialized

Signed-off-by: Tobias Rapp <t.rapp@noa-archive.com>
---
 ffmpeg.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ffmpeg.c b/ffmpeg.c
index c47a824..e4890a4 100644
--- a/ffmpeg.c
+++ b/ffmpeg.c
@@ -2070,6 +2070,7 @@  static int send_frame_to_filters(InputStream *ist, AVFrame *decoded_frame)
     int i, ret;
     AVFrame *f;
 
+    av_assert1(ist->nb_filters > 0); /* ensure ret is initialized */
     for (i = 0; i < ist->nb_filters; i++) {
         if (i < ist->nb_filters - 1) {
             f = ist->filter_frame;
-- 
1.9.1