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

[Andreas Cadhalpun]

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

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Andreas Cadhalpun]

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  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

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Andreas Cadhalpun]

On 26.10.2016 20:15, Paul B Mahol wrote:
> On 10/25/16, Michael Niedermayer  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

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Paul B Mahol]

On 10/25/16, Michael Niedermayer  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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

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


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Andreas Cadhalpun]

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


___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Paul B Mahol]

On 10/23/16, Andreas Cadhalpun  wrote:
> A negative sample rate doesn't make sense and triggers assertions in
> av_rescale_rnd.
>
> Signed-off-by: Andreas Cadhalpun 
> ---
>  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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Andreas Cadhalpun]

A negative sample rate doesn't make sense and triggers assertions in
av_rescale_rnd.

Signed-off-by: Andreas Cadhalpun 
---
 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