| Message ID | 6622a3c7-3900-8982-1179-4ccdd0fec2b3@googlemail.com |
|---|---|
| State | Accepted |
| Commit | 2eb05eaa682ec49eade91e358ace4e1415695686 |
| Headers | show |
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.
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
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 [...]
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.
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
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
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 --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;
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(+)