Re: [FFmpeg-devel] [PATCH] pngdec: add ability to check chunk CRC

[Michael Niedermayer]

On Fri, Mar 08, 2019 at 03:15:40PM +0100, Lynne wrote:
> 7 Mar 2019, 21:22 by mich...@niedermayer.cc:
> 
> > On Thu, Mar 07, 2019 at 07:26:32PM +0100, Lynne wrote:
> >
> >> By default now, if AV_EF_CRCCHECK or AV_EF_IGNORE_ERR are enabled the 
> >> decoder
> >> will skip the chunk and carry on with the next one. This should make the   
> >> 
> >> decoder able to decode more corrupt files because the functions which 
> >> decode
> >> individual chunks will very likely error out if fed invalid data and stop 
> >> the
> >> decoding of the entire image.
> >> Should this be made default? CRC verification doesn't take long even for 
> >> very
> >> large files.  
> >> Also fix the length check for chunk size. It needs to take into account the
> >> 4 byte tag as well as the 4 byte CRC.
> >>
> >> pngdec.c |   19 ++-
> >>  1 file changed, 18 insertions(+), 1 deletion(-)
> >> 4255c91468cee2bc2fa757fae69762ff5ee5774a  
> >> 0001-pngdec-add-ability-to-check-chunk-CRC.patch
> >> From 7aff99d12faf557753c5ee860a9672c7a09a26e3 Mon Sep 17 00:00:00 2001
> >> From: Lynne <>> d...@lynne.ee >> >
> >> Date: Thu, 7 Mar 2019 18:15:23 +
> >> Subject: [PATCH] pngdec: add ability to check chunk CRC
> >>
> >> By default now, if AV_EF_CRCCHECK or AV_EF_IGNORE_ERR are enabled the 
> >> decoder
> >> will skip the chunk and carry on with the next one. This should make the
> >> decoder able to decode more corrupt files because the functions which 
> >> decode
> >> individual chunks will very likely error out if fed invalid data and stop 
> >> the
> >> decoding of the entire image.
> >> Should this be made default? CRC verification doesn't take long even for 
> >> very
> >> large files.
> >>
> >
> > i would tend toward enabling it by default but maybe first post some
> > numbers of how much this changes decode time 
> >
> 
> For the largest png I found: 
> https://vk.com/doc218587497_437472325?hash=51300ca9ba40f462ac=1bcf9a57b0d989da1f
>  
> 
> 
> There was no increase in decoding time, it took 2.3 seconds on my machine
> with and without -err_detect crccheck.
> 
> With -err_detect crccheck perf reported 2.52% spent in av_crc
> 
> 
> >> Also fix the length check for chunk size. It needs to take into account the
> >> 4 byte tag as well as the 4 byte CRC.
> >>
> >
> > this should be a seperate patch as its unrelated
> >
> 
> removed and attached new patch file
> 
> Maybe always enabling the CRC check isn't worth it since if you download from
> the internet you could either get a full error-free file or an incomplete one
> rather than a corrupt one. Maybe only for torrents with huge png files where
> not all chunks have been downloaded yet, or broken hard drives, but the 
> ignore_err
> flag could be manually enabled in those cases.

>  pngdec.c |   17 +
>  1 file changed, 17 insertions(+)
> 57ccb0b81aeca1e08bc0f1475c048007d7f32c26  
> 0001-pngdec-add-ability-to-check-chunk-CRC.patch
> From b911d3cb36828ad3c910ca6bf8b96a58ce398191 Mon Sep 17 00:00:00 2001
> From: Lynne 
> Date: Thu, 7 Mar 2019 18:15:23 +
> Subject: [PATCH] pngdec: add ability to check chunk CRC
> 
> By default now, if AV_EF_CRCCHECK or AV_EF_IGNORE_ERR are enabled the decoder
> will skip the chunk and carry on with the next one. This should make the
> decoder able to decode more corrupt files because the functions which decode
> individual chunks will very likely error out if fed invalid data and stop the
> decoding of the entire image.
> ---
>  libavcodec/pngdec.c | 17 +
>  1 file changed, 17 insertions(+)
> 
> diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> index 189bb9a4c1..9743f1cb76 100644
> --- a/libavcodec/pngdec.c
> +++ b/libavcodec/pngdec.c
> @@ -23,6 +23,7 @@
>  
>  #include "libavutil/avassert.h"
>  #include "libavutil/bprint.h"
> +#include "libavutil/crc.h"
>  #include "libavutil/imgutils.h"
>  #include "libavutil/stereo3d.h"
>  #include "libavutil/mastering_display_metadata.h"
> @@ -1169,6 +1170,7 @@ static int handle_p_frame_apng(AVCodecContext *avctx, 
> PNGDecContext *s,
>  static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
> AVFrame *p, AVPacket *avpkt)
>  {
> +const AVCRC *crc_tab = av_crc_get_table(AV_CRC_32_IEEE_LE);
>  AVDictionary **metadatap = NULL;
>  uint32_t tag, length;
>  int decode_next_dat = 0;
> @@ -1203,6 +1205,21 @@ static int decode_frame_common(AVCodecContext *avctx, 
> PNGDecContext *s,
>  ret = AVERROR_INVALIDDATA;
>  goto fail;
>  }
> +if (avctx->err_recognition & (AV_EF_CRCCHECK | AV_EF_IGNORE_ERR)) {

The usage of AV_EF_IGNORE_ERR is incorrect.
the code does the opposite of what the flag means
what the code does is handle errors when the flag says dont


> +uint32_t crc_sig = AV_RB32(s->gb.buffer 

Re: [FFmpeg-devel] [PATCH] pngdec: add ability to check chunk CRC

[Lynne]

8 Mar 2019, 14:15 by d...@lynne.ee:

> 7 Mar 2019, 21:22 by > mich...@niedermayer.cc 
> > :
>
>> On Thu, Mar 07, 2019 at 07:26:32PM +0100, Lynne wrote:
>>
>>> By default now, if AV_EF_CRCCHECK or AV_EF_IGNORE_ERR are enabled the 
>>> decoder
>>> will skip the chunk and carry on with the next one. This should make the
>>>    
>>> decoder able to decode more corrupt files because the functions which decode
>>> individual chunks will very likely error out if fed invalid data and stop 
>>> the
>>> decoding of the entire image.
>>> Should this be made default? CRC verification doesn't take long even for 
>>> very
>>> large files.  
>>> Also fix the length check for chunk size. It needs to take into account the
>>> 4 byte tag as well as the 4 byte CRC.
>>>
>>> pngdec.c |   19 ++-
>>>  1 file changed, 18 insertions(+), 1 deletion(-)
>>> 4255c91468cee2bc2fa757fae69762ff5ee5774a  
>>> 0001-pngdec-add-ability-to-check-chunk-CRC.patch
>>> From 7aff99d12faf557753c5ee860a9672c7a09a26e3 Mon Sep 17 00:00:00 2001
>>> From: Lynne <>> >>> d...@lynne.ee > 
>>> d...@lynne.ee >> >
>>> Date: Thu, 7 Mar 2019 18:15:23 +
>>> Subject: [PATCH] pngdec: add ability to check chunk CRC
>>>
>>> By default now, if AV_EF_CRCCHECK or AV_EF_IGNORE_ERR are enabled the 
>>> decoder
>>> will skip the chunk and carry on with the next one. This should make the
>>> decoder able to decode more corrupt files because the functions which decode
>>> individual chunks will very likely error out if fed invalid data and stop 
>>> the
>>> decoding of the entire image.
>>> Should this be made default? CRC verification doesn't take long even for 
>>> very
>>> large files.
>>>
>>
>> i would tend toward enabling it by default but maybe first post some
>> numbers of how much this changes decode time 
>>
>
> For the largest png I found: > 
> https://vk.com/doc218587497_437472325?hash=51300ca9ba40f462ac=1bcf9a57b0d989da1f
>  
> >
>   <> 
> https://vk.com/doc218587497_437472325?hash=51300ca9ba40f462ac=1bcf9a57b0d989da1f
>  
> >
>  >
>
> There was no increase in decoding time, it took 2.3 seconds on my machine
> with and without -err_detect crccheck.
>
> With -err_detect crccheck perf reported 2.52% spent in av_crc
>
>
>>> Also fix the length check for chunk size. It needs to take into account the
>>> 4 byte tag as well as the 4 byte CRC.
>>>
>>
>> this should be a seperate patch as its unrelated
>>
>
> removed and attached new patch file
>
> Maybe always enabling the CRC check isn't worth it since if you download from
> the internet you could either get a full error-free file or an incomplete one
> rather than a corrupt one. Maybe only for torrents with huge png files where
> not all chunks have been downloaded yet, or broken hard drives, but the 
> ignore_err
> flag could be manually enabled in those cases.
>

Ping? The apedec CRC patch needs a review too.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] pngdec: add ability to check chunk CRC

[Lynne]

7 Mar 2019, 21:22 by mich...@niedermayer.cc:

> On Thu, Mar 07, 2019 at 07:26:32PM +0100, Lynne wrote:
>
>> By default now, if AV_EF_CRCCHECK or AV_EF_IGNORE_ERR are enabled the decoder
>> will skip the chunk and carry on with the next one. This should make the 
>>   
>> decoder able to decode more corrupt files because the functions which decode
>> individual chunks will very likely error out if fed invalid data and stop the
>> decoding of the entire image.
>> Should this be made default? CRC verification doesn't take long even for very
>> large files.  
>> Also fix the length check for chunk size. It needs to take into account the
>> 4 byte tag as well as the 4 byte CRC.
>>
>> pngdec.c |   19 ++-
>>  1 file changed, 18 insertions(+), 1 deletion(-)
>> 4255c91468cee2bc2fa757fae69762ff5ee5774a  
>> 0001-pngdec-add-ability-to-check-chunk-CRC.patch
>> From 7aff99d12faf557753c5ee860a9672c7a09a26e3 Mon Sep 17 00:00:00 2001
>> From: Lynne <>> d...@lynne.ee >> >
>> Date: Thu, 7 Mar 2019 18:15:23 +
>> Subject: [PATCH] pngdec: add ability to check chunk CRC
>>
>> By default now, if AV_EF_CRCCHECK or AV_EF_IGNORE_ERR are enabled the decoder
>> will skip the chunk and carry on with the next one. This should make the
>> decoder able to decode more corrupt files because the functions which decode
>> individual chunks will very likely error out if fed invalid data and stop the
>> decoding of the entire image.
>> Should this be made default? CRC verification doesn't take long even for very
>> large files.
>>
>
> i would tend toward enabling it by default but maybe first post some
> numbers of how much this changes decode time 
>

For the largest png I found: 
https://vk.com/doc218587497_437472325?hash=51300ca9ba40f462ac=1bcf9a57b0d989da1f
 


There was no increase in decoding time, it took 2.3 seconds on my machine
with and without -err_detect crccheck.

With -err_detect crccheck perf reported 2.52% spent in av_crc


>> Also fix the length check for chunk size. It needs to take into account the
>> 4 byte tag as well as the 4 byte CRC.
>>
>
> this should be a seperate patch as its unrelated
>

removed and attached new patch file

Maybe always enabling the CRC check isn't worth it since if you download from
the internet you could either get a full error-free file or an incomplete one
rather than a corrupt one. Maybe only for torrents with huge png files where
not all chunks have been downloaded yet, or broken hard drives, but the 
ignore_err
flag could be manually enabled in those cases.
>From b911d3cb36828ad3c910ca6bf8b96a58ce398191 Mon Sep 17 00:00:00 2001
From: Lynne 
Date: Thu, 7 Mar 2019 18:15:23 +
Subject: [PATCH] pngdec: add ability to check chunk CRC

By default now, if AV_EF_CRCCHECK or AV_EF_IGNORE_ERR are enabled the decoder
will skip the chunk and carry on with the next one. This should make the
decoder able to decode more corrupt files because the functions which decode
individual chunks will very likely error out if fed invalid data and stop the
decoding of the entire image.
---
 libavcodec/pngdec.c | 17 +
 1 file changed, 17 insertions(+)

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index 189bb9a4c1..9743f1cb76 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -23,6 +23,7 @@
 
 #include "libavutil/avassert.h"
 #include "libavutil/bprint.h"
+#include "libavutil/crc.h"
 #include "libavutil/imgutils.h"
 #include "libavutil/stereo3d.h"
 #include "libavutil/mastering_display_metadata.h"
@@ -1169,6 +1170,7 @@ static int handle_p_frame_apng(AVCodecContext *avctx, PNGDecContext *s,
 static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
AVFrame *p, AVPacket *avpkt)
 {
+const AVCRC *crc_tab = av_crc_get_table(AV_CRC_32_IEEE_LE);
 AVDictionary **metadatap = NULL;
 uint32_t tag, length;
 int decode_next_dat = 0;
@@ -1203,6 +1205,21 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
 ret = AVERROR_INVALIDDATA;
 goto fail;
 }
+if (avctx->err_recognition & (AV_EF_CRCCHECK | AV_EF_IGNORE_ERR)) {
+uint32_t crc_sig = AV_RB32(s->gb.buffer + length + 4);
+uint32_t crc_cal = ~av_crc(crc_tab, UINT32_MAX, s->gb.buffer, length + 4);
+if (crc_sig ^ crc_cal) {
+av_log(avctx, AV_LOG_ERROR, "CRC mismatch in chunk");
+if (avctx->err_recognition & AV_EF_EXPLODE) {
+av_log(avctx, AV_LOG_ERROR, ", quitting\n");
+ret = AVERROR_INVALIDDATA;
+goto fail;
+}
+av_log(avctx, AV_LOG_ERROR, ", skipping\n");
+bytestream2_skip(>gb, 4); /* tag */
+goto skip_tag;
+}
+}
 tag = 

Re: [FFmpeg-devel] [PATCH] pngdec: add ability to check chunk CRC

[Michael Niedermayer]

On Thu, Mar 07, 2019 at 07:26:32PM +0100, Lynne wrote:
> By default now, if AV_EF_CRCCHECK or AV_EF_IGNORE_ERR are enabled the decoder
> will skip the chunk and carry on with the next one. This should make the  
>  
> decoder able to decode more corrupt files because the functions which decode
> individual chunks will very likely error out if fed invalid data and stop the
> decoding of the entire image.
> Should this be made default? CRC verification doesn't take long even for very
> large files.  
> Also fix the length check for chunk size. It needs to take into account the
> 4 byte tag as well as the 4 byte CRC.
> 

>  pngdec.c |   19 ++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 4255c91468cee2bc2fa757fae69762ff5ee5774a  
> 0001-pngdec-add-ability-to-check-chunk-CRC.patch
> From 7aff99d12faf557753c5ee860a9672c7a09a26e3 Mon Sep 17 00:00:00 2001
> From: Lynne 
> Date: Thu, 7 Mar 2019 18:15:23 +
> Subject: [PATCH] pngdec: add ability to check chunk CRC
> 
> By default now, if AV_EF_CRCCHECK or AV_EF_IGNORE_ERR are enabled the decoder
> will skip the chunk and carry on with the next one. This should make the
> decoder able to decode more corrupt files because the functions which decode
> individual chunks will very likely error out if fed invalid data and stop the
> decoding of the entire image.
> Should this be made default? CRC verification doesn't take long even for very
> large files.

i would tend toward enabling it by default but maybe first post some
numbers of how much this changes decode time 


> Also fix the length check for chunk size. It needs to take into account the
> 4 byte tag as well as the 4 byte CRC.

this should be a seperate patch as its unrelated

thx

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In fact, the RIAA has been known to suggest that students drop out
of college or go to community college in order to be able to afford
settlements. -- The RIAA


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

[FFmpeg-devel] [PATCH] pngdec: add ability to check chunk CRC

[Lynne]

By default now, if AV_EF_CRCCHECK or AV_EF_IGNORE_ERR are enabled the decoder
will skip the chunk and carry on with the next one. This should make the   
decoder able to decode more corrupt files because the functions which decode
individual chunks will very likely error out if fed invalid data and stop the
decoding of the entire image.
Should this be made default? CRC verification doesn't take long even for very
large files.  
Also fix the length check for chunk size. It needs to take into account the
4 byte tag as well as the 4 byte CRC.

>From 7aff99d12faf557753c5ee860a9672c7a09a26e3 Mon Sep 17 00:00:00 2001
From: Lynne 
Date: Thu, 7 Mar 2019 18:15:23 +
Subject: [PATCH] pngdec: add ability to check chunk CRC

By default now, if AV_EF_CRCCHECK or AV_EF_IGNORE_ERR are enabled the decoder
will skip the chunk and carry on with the next one. This should make the
decoder able to decode more corrupt files because the functions which decode
individual chunks will very likely error out if fed invalid data and stop the
decoding of the entire image.
Should this be made default? CRC verification doesn't take long even for very
large files.
Also fix the length check for chunk size. It needs to take into account the
4 byte tag as well as the 4 byte CRC.
---
 libavcodec/pngdec.c | 19 ++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index 189bb9a4c1..30b6dc3166 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -23,6 +23,7 @@
 
 #include "libavutil/avassert.h"
 #include "libavutil/bprint.h"
+#include "libavutil/crc.h"
 #include "libavutil/imgutils.h"
 #include "libavutil/stereo3d.h"
 #include "libavutil/mastering_display_metadata.h"
@@ -1169,6 +1170,7 @@ static int handle_p_frame_apng(AVCodecContext *avctx, PNGDecContext *s,
 static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
AVFrame *p, AVPacket *avpkt)
 {
+const AVCRC *crc_tab = av_crc_get_table(AV_CRC_32_IEEE_LE);
 AVDictionary **metadatap = NULL;
 uint32_t tag, length;
 int decode_next_dat = 0;
@@ -1198,11 +1200,26 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
 }
 
 length = bytestream2_get_be32(>gb);
-if (length > 0x7fff || length > bytestream2_get_bytes_left(>gb)) {
+if (length > 0x7fff || (length + 8) > bytestream2_get_bytes_left(>gb)) {
 av_log(avctx, AV_LOG_ERROR, "chunk too big\n");
 ret = AVERROR_INVALIDDATA;
 goto fail;
 }
+if (avctx->err_recognition & (AV_EF_CRCCHECK | AV_EF_IGNORE_ERR)) {
+uint32_t crc_sig = AV_RB32(s->gb.buffer + length + 4);
+uint32_t crc_cal = ~av_crc(crc_tab, UINT32_MAX, s->gb.buffer, length + 4);
+if (crc_sig ^ crc_cal) {
+av_log(avctx, AV_LOG_ERROR, "CRC mismatch in chunk");
+if (avctx->err_recognition & AV_EF_EXPLODE) {
+av_log(avctx, AV_LOG_ERROR, ", quitting\n");
+ret = AVERROR_INVALIDDATA;
+goto fail;
+}
+av_log(avctx, AV_LOG_ERROR, ", skipping\n");
+bytestream2_skip(>gb, 4); /* tag */
+goto skip_tag;
+}
+}
 tag = bytestream2_get_le32(>gb);
 if (avctx->debug & FF_DEBUG_STARTCODE)
 av_log(avctx, AV_LOG_DEBUG, "png: tag=%s length=%u\n",
-- 
2.21.0

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