[FFmpeg-devel] avcodec/flac: check frame header crc only if requested

Submitted by James Almer on Dec. 8, 2016, 3:33 a.m.

Details

Message ID 20161208033356.1716-1-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer Dec. 8, 2016, 3:33 a.m.
It's more consistent with other similar checks in the decoder, and should
help with fuzzing.

Signed-off-by: James Almer <jamrial@gmail.com>
---
I could also add an AV_EF_EXPLODE check before aborting, but i figured
that with a frame header crc failure it's pretty much guaranteed the 
file will be unplayable.

 libavcodec/flac.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Michael Niedermayer Dec. 8, 2016, 12:31 p.m.
On Thu, Dec 08, 2016 at 12:33:56AM -0300, James Almer wrote:
> It's more consistent with other similar checks in the decoder, and should
> help with fuzzing.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> I could also add an AV_EF_EXPLODE check before aborting, but i figured
> that with a frame header crc failure it's pretty much guaranteed the 
> file will be unplayable.
> 
>  libavcodec/flac.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

FFmpeg decoders primary usecase is to decode for human consumption
for this producing the best quality possible and doing so fast is
the goal.
The default thus should be to do checks that improve quality
its probably better if fuzzers disable these checks instead of the
"default being changed"

treating damaged headers as if they are valid will lead to bad
quality at the location where this occurs.
also this function is used by the parser and i _think_ the code expects
that it always checks the crc.
also for detecting the parameters of a stream, its important to use
a undamaged header otherwise the parameters could be more often wrong.

whatever default is used for err_recognition some checks like this
one should be enabled in that case
actually i thought we have a non zero default but it seems 0 in
libavcodec, i think this is bad as well.
With a 0 default nothing can be enabled by default (if the mask is all
positive with 0 being all disabled)

[...]
James Almer Dec. 8, 2016, 5:45 p.m.
On 12/8/2016 9:31 AM, Michael Niedermayer wrote:
> FFmpeg decoders primary usecase is to decode for human consumption
> for this producing the best quality possible and doing so fast is
> the goal.
> The default thus should be to do checks that improve quality
> its probably better if fuzzers disable these checks instead of the
> "default being changed"

This patch is needed for fuzzers to actually disable the check.
I assume there's a high chance for fuzzers to change some bits on each
frame header, and if the decoder aborts unconditionally right then and
there then the fuzzing is unlikely to ever find any real bug in the code.

If a real file has a damaged frame header, then even if it doesn't abort
at that point it will fail somewhere down the line anyway, due to a bogus
channel count or framesize value.

> 
> treating damaged headers as if they are valid will lead to bad
> quality at the location where this occurs.
> also this function is used by the parser and i _think_ the code expects
> that it always checks the crc.

If this is true then this patch is not good.

> also for detecting the parameters of a stream, its important to use
> a undamaged header otherwise the parameters could be more often wrong.
> 
> whatever default is used for err_recognition some checks like this
> one should be enabled in that case
> actually i thought we have a non zero default but it seems 0 in
> libavcodec, i think this is bad as well.
> With a 0 default nothing can be enabled by default (if the mask is all
> positive with 0 being all disabled)

A couple years ago crccheck was the default value for err_recognition,
but you sent a patch to disable it since it was affecting hevc decoding
performance. I don't think that has changed since then.

Do we have a way to add codec dependent defaults for options like
err_recognition? lossless codecs could then have crc enabled by default
and lossy codecs have it disabled, for example.

> 
> [...]
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Dave Rice Dec. 8, 2016, 7:34 p.m.
> On Dec 8, 2016, at 12:45 PM, James Almer <jamrial@gmail.com> wrote:
> 
> On 12/8/2016 9:31 AM, Michael Niedermayer wrote:
>> FFmpeg decoders primary usecase is to decode for human consumption
>> for this producing the best quality possible and doing so fast is
>> the goal.
>> The default thus should be to do checks that improve quality
>> its probably better if fuzzers disable these checks instead of the
>> "default being changed"
> 
> This patch is needed for fuzzers to actually disable the check.
> I assume there's a high chance for fuzzers to change some bits on each
> frame header, and if the decoder aborts unconditionally right then and
> there then the fuzzing is unlikely to ever find any real bug in the code.
> 
> If a real file has a damaged frame header, then even if it doesn't abort
> at that point it will fail somewhere down the line anyway, due to a bogus
> channel count or framesize value.
> 
>> 
>> treating damaged headers as if they are valid will lead to bad
>> quality at the location where this occurs.
>> also this function is used by the parser and i _think_ the code expects
>> that it always checks the crc.
> 
> If this is true then this patch is not good.
> 
>> also for detecting the parameters of a stream, its important to use
>> a undamaged header otherwise the parameters could be more often wrong.
>> 
>> whatever default is used for err_recognition some checks like this
>> one should be enabled in that case
>> actually i thought we have a non zero default but it seems 0 in
>> libavcodec, i think this is bad as well.
>> With a 0 default nothing can be enabled by default (if the mask is all
>> positive with 0 being all disabled)
> 
> A couple years ago crccheck was the default value for err_recognition,
> but you sent a patch to disable it since it was affecting hevc decoding
> performance. I don't think that has changed since then.
> 
> Do we have a way to add codec dependent defaults for options like
> err_recognition? lossless codecs could then have crc enabled by default
> and lossy codecs have it disabled, for example.

+1

>> [...]
>> 
>> 
>> 
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Michael Niedermayer Dec. 9, 2016, 1:16 a.m.
On Thu, Dec 08, 2016 at 02:45:42PM -0300, James Almer wrote:
> On 12/8/2016 9:31 AM, Michael Niedermayer wrote:
> > FFmpeg decoders primary usecase is to decode for human consumption
> > for this producing the best quality possible and doing so fast is
> > the goal.
> > The default thus should be to do checks that improve quality
> > its probably better if fuzzers disable these checks instead of the
> > "default being changed"
> 
> This patch is needed for fuzzers to actually disable the check.
> I assume there's a high chance for fuzzers to change some bits on each
> frame header, and if the decoder aborts unconditionally right then and
> there then the fuzzing is unlikely to ever find any real bug in the code.
> 
> If a real file has a damaged frame header, then even if it doesn't abort
> at that point it will fail somewhere down the line anyway, due to a bogus
> channel count or framesize value.
> 
> > 
> > treating damaged headers as if they are valid will lead to bad
> > quality at the location where this occurs.
> > also this function is used by the parser and i _think_ the code expects
> > that it always checks the crc.
> 
> If this is true then this patch is not good.
> 
> > also for detecting the parameters of a stream, its important to use
> > a undamaged header otherwise the parameters could be more often wrong.
> > 
> > whatever default is used for err_recognition some checks like this
> > one should be enabled in that case
> > actually i thought we have a non zero default but it seems 0 in
> > libavcodec, i think this is bad as well.
> > With a 0 default nothing can be enabled by default (if the mask is all
> > positive with 0 being all disabled)
> 
> A couple years ago crccheck was the default value for err_recognition,
> but you sent a patch to disable it since it was affecting hevc decoding
> performance. I don't think that has changed since then.
> 

> Do we have a way to add codec dependent defaults for options like
> err_recognition? lossless codecs could then have crc enabled by default
> and lossy codecs have it disabled, for example.

AV_EF_CAREFUL is documented as
"///< consider things that violate the spec, are fast to calculate and have not been seen in the wild as errors"

id say this should be enabled by default and used for cases like this

if its not suitable for some reason, then we are missing some value
in the enum for this case.

the enum is in that case a bit like a language missing words to
describe what is actually meant.

also codecs can have CRCs on both headers and the data itself,
headers being small should always be fast to check, data itself may
be slow to check. a all on vs off could be insufficient for a
decoder.

[...]

Patch hide | download patch | download mbox

diff --git a/libavcodec/flac.c b/libavcodec/flac.c
index 5ffbf93..8e1b059 100644
--- a/libavcodec/flac.c
+++ b/libavcodec/flac.c
@@ -135,7 +135,8 @@  int ff_flac_decode_frame_header(AVCodecContext *avctx, GetBitContext *gb,
 
     /* header CRC-8 check */
     skip_bits(gb, 8);
-    if (av_crc(av_crc_get_table(AV_CRC_8_ATM), 0, gb->buffer,
+    if ((avctx->err_recognition & (AV_EF_CRCCHECK|AV_EF_COMPLIANT)) &&
+        av_crc(av_crc_get_table(AV_CRC_8_ATM), 0, gb->buffer,
                get_bits_count(gb)/8)) {
         av_log(avctx, AV_LOG_ERROR + log_level_offset,
                "header crc mismatch\n");