diff mbox

[FFmpeg-devel,01/12] adxdec: validate sample_rate

Message ID 6622a3c7-3900-8982-1179-4ccdd0fec2b3@googlemail.com
State Accepted
Commit 2eb05eaa682ec49eade91e358ace4e1415695686
Headers show

Commit Message

Andreas Cadhalpun Oct. 23, 2016, 4:26 p.m. UTC
A negative sample rate doesn't make sense and triggers assertions in
av_rescale_rnd.

Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
---
 libavformat/adxdec.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Paul B Mahol Oct. 25, 2016, 10:58 a.m. UTC | #1
On 10/23/16, Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> wrote:
> A negative sample rate doesn't make sense and triggers assertions in
> av_rescale_rnd.
>
> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> ---
>  libavformat/adxdec.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/libavformat/adxdec.c b/libavformat/adxdec.c
> index cf44531..0315ecb 100644
> --- a/libavformat/adxdec.c
> +++ b/libavformat/adxdec.c
> @@ -109,6 +109,11 @@ static int adx_read_header(AVFormatContext *s)
>          return AVERROR_INVALIDDATA;
>      }
>
> +    if (par->sample_rate <= 0) {
> +        av_log(s, AV_LOG_ERROR, "Invalid sample rate %d\n",
> par->sample_rate);
> +        return AVERROR_INVALIDDATA;
> +    }
> +
>      par->codec_type  = AVMEDIA_TYPE_AUDIO;
>      par->codec_id    = s->iformat->raw_codec_id;
>      par->bit_rate    = par->sample_rate * par->channels * BLOCK_SIZE * 8LL
> / BLOCK_SAMPLES;
> --
> 2.9.3
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

patch(es)have good intent, but better fix is doing/checking it in single place.
Andreas Cadhalpun Oct. 25, 2016, 5:45 p.m. UTC | #2
On 25.10.2016 12:58, Paul B Mahol wrote:
> patch(es)have good intent, but better fix is doing/checking it in single place.

I don't agree.
In general, validity checks should be where the values are actually read.
This eliminates the risk that bogus values could cause problems between being set
and being checked.
Also, having only a check in a central place is bad for debugging, because it is
not immediately clear where the bogus value came from, when the check is triggered.
(I know this from personal experience debugging all the cases triggering the
assert in av_rescale_rnd.)

The problem with that approach is that such checks can easily be forgotten, which
is why I think a check in a central place would make sense in addition to checking
the individual cases.

Best regards,
Andreas
Michael Niedermayer Oct. 25, 2016, 9:27 p.m. UTC | #3
On Tue, Oct 25, 2016 at 07:45:25PM +0200, Andreas Cadhalpun wrote:
> On 25.10.2016 12:58, Paul B Mahol wrote:
> > patch(es)have good intent, but better fix is doing/checking it in single place.
> 
> I don't agree.
> In general, validity checks should be where the values are actually read.
> This eliminates the risk that bogus values could cause problems between being set
> and being checked.
> Also, having only a check in a central place is bad for debugging, because it is
> not immediately clear where the bogus value came from, when the check is triggered.
> (I know this from personal experience debugging all the cases triggering the
> assert in av_rescale_rnd.)
> 
> The problem with that approach is that such checks can easily be forgotten, which
> is why I think a check in a central place would make sense in addition to checking
> the individual cases.

some formats may also lack a sample rate like mpeg ps/ts
the fact is known insude the demuxer, generic code after it doesnt
know. Doing the only check after the parser is a bit late OTOH

[...]
Paul B Mahol Oct. 26, 2016, 6:15 p.m. UTC | #4
On 10/25/16, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Tue, Oct 25, 2016 at 07:45:25PM +0200, Andreas Cadhalpun wrote:
>> On 25.10.2016 12:58, Paul B Mahol wrote:
>> > patch(es)have good intent, but better fix is doing/checking it in single
>> > place.
>>
>> I don't agree.
>> In general, validity checks should be where the values are actually read.
>> This eliminates the risk that bogus values could cause problems between
>> being set
>> and being checked.
>> Also, having only a check in a central place is bad for debugging, because
>> it is
>> not immediately clear where the bogus value came from, when the check is
>> triggered.
>> (I know this from personal experience debugging all the cases triggering
>> the
>> assert in av_rescale_rnd.)
>>
>> The problem with that approach is that such checks can easily be
>> forgotten, which
>> is why I think a check in a central place would make sense in addition to
>> checking
>> the individual cases.
>
> some formats may also lack a sample rate like mpeg ps/ts
> the fact is known insude the demuxer, generic code after it doesnt
> know. Doing the only check after the parser is a bit late OTOH
>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> What does censorship reveal? It reveals fear. -- Julian Assange
>

I'm not (yet) aware of better "fix" so do as you like.
Andreas Cadhalpun Oct. 26, 2016, 7:44 p.m. UTC | #5
On 26.10.2016 20:15, Paul B Mahol wrote:
> On 10/25/16, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> On Tue, Oct 25, 2016 at 07:45:25PM +0200, Andreas Cadhalpun wrote:
>>> On 25.10.2016 12:58, Paul B Mahol wrote:
>>>> patch(es)have good intent, but better fix is doing/checking it in single
>>>> place.
>>>
>>> I don't agree.
>>> In general, validity checks should be where the values are actually read.
>>> This eliminates the risk that bogus values could cause problems between
>>> being set
>>> and being checked.
>>> Also, having only a check in a central place is bad for debugging, because
>>> it is
>>> not immediately clear where the bogus value came from, when the check is
>>> triggered.
>>> (I know this from personal experience debugging all the cases triggering
>>> the
>>> assert in av_rescale_rnd.)
>>>
>>> The problem with that approach is that such checks can easily be
>>> forgotten, which
>>> is why I think a check in a central place would make sense in addition to
>>> checking
>>> the individual cases.
>>
>> some formats may also lack a sample rate like mpeg ps/ts
>> the fact is known insude the demuxer, generic code after it doesnt
>> know. Doing the only check after the parser is a bit late OTOH
>>
>> [...]
>>
>> --
>> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>
>> What does censorship reveal? It reveals fear. -- Julian Assange
>>
> 
> I'm not (yet) aware of better "fix" so do as you like.

Have you seen the patch for checking this in a central place [1]?
It would catch all the negative sample rates, but not the negative
timescales.
(I still think it should be checked both centrally and locally.)

Best regards,
Andreas


1: https://ffmpeg.org/pipermail/ffmpeg-devel/2016-October/201769.html
Andreas Cadhalpun Nov. 2, 2016, 10:09 p.m. UTC | #6
On 26.10.2016 21:44, Andreas Cadhalpun wrote:
> On 26.10.2016 20:15, Paul B Mahol wrote:
>> On 10/25/16, Michael Niedermayer <michael@niedermayer.cc> wrote:
>>> On Tue, Oct 25, 2016 at 07:45:25PM +0200, Andreas Cadhalpun wrote:
>>>> On 25.10.2016 12:58, Paul B Mahol wrote:
>>>>> patch(es)have good intent, but better fix is doing/checking it in single
>>>>> place.
>>>>
>>>> I don't agree.
>>>> In general, validity checks should be where the values are actually read.
>>>> This eliminates the risk that bogus values could cause problems between
>>>> being set
>>>> and being checked.
>>>> Also, having only a check in a central place is bad for debugging, because
>>>> it is
>>>> not immediately clear where the bogus value came from, when the check is
>>>> triggered.
>>>> (I know this from personal experience debugging all the cases triggering
>>>> the
>>>> assert in av_rescale_rnd.)
>>>>
>>>> The problem with that approach is that such checks can easily be
>>>> forgotten, which
>>>> is why I think a check in a central place would make sense in addition to
>>>> checking
>>>> the individual cases.
>>>
>>> some formats may also lack a sample rate like mpeg ps/ts
>>> the fact is known insude the demuxer, generic code after it doesnt
>>> know. Doing the only check after the parser is a bit late OTOH
>>>
>>> [...]
>>>
>>> --
>>> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>>
>>> What does censorship reveal? It reveals fear. -- Julian Assange
>>>
>>
>> I'm not (yet) aware of better "fix" so do as you like.
> 
> Have you seen the patch for checking this in a central place [1]?
> It would catch all the negative sample rates, but not the negative
> timescales.
> (I still think it should be checked both centrally and locally.)

In the absence of further comments, I intend to push this set in a few days.

Best regards,
Andreas
Andreas Cadhalpun Nov. 6, 2016, 11:54 p.m. UTC | #7
On 02.11.2016 23:09, Andreas Cadhalpun wrote:
> In the absence of further comments, I intend to push this set in a few days.

I've pushed this now.

Best regards,
Andreas
diff mbox

Patch

diff --git a/libavformat/adxdec.c b/libavformat/adxdec.c
index cf44531..0315ecb 100644
--- a/libavformat/adxdec.c
+++ b/libavformat/adxdec.c
@@ -109,6 +109,11 @@  static int adx_read_header(AVFormatContext *s)
         return AVERROR_INVALIDDATA;
     }
 
+    if (par->sample_rate <= 0) {
+        av_log(s, AV_LOG_ERROR, "Invalid sample rate %d\n", par->sample_rate);
+        return AVERROR_INVALIDDATA;
+    }
+
     par->codec_type  = AVMEDIA_TYPE_AUDIO;
     par->codec_id    = s->iformat->raw_codec_id;
     par->bit_rate    = par->sample_rate * par->channels * BLOCK_SIZE * 8LL / BLOCK_SAMPLES;