[FFmpeg-devel] [PATCH v2] avcodec/h2645_parse: Don't treat 0x000002 as a start code and truncate
According to ITU-T H.265 7.4.2.1 this byte sequence should not appear in a
NAL unit but in practice in rare cases it seems it does, possibly due to buggy
encoders. Other players like VLC and Quicktime seem to be fine with it.
Currently when this sequence is found it is treated as if the next start code
has been found and the NAL unit gets truncated.
This change limits the code to only look for first start code 0x001 or
first escape 0x03.
Sadly i can't share the original source file with the issue but the first
80 bytes of the NAL unit looks like this:
│00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f│0123456789abcdef│
0x0│00 00 00 01 02 01 d0 bc 57 a1 b8 44 70 01 00 0b│W..Dp...│
0x00010│80 2e 00 c2 6c ec 3e b9 e3 03 fb 91 2e d2 43 cb│l.>...C.│
0x00020│1d 2c 00 00 02 00 02 00 5c 93 72 6f 31 76 18 00│.,..\.ro1v..│
0x00030│08 38 aa b1 4c 33 3f fd 08 cb 77 9b d4 3c db 02│.8..L3?...w..<..│
0x00040│a2 04 73 15 75 de 3b c4 67 c0 8f ca ad 31 f1 99│..s.u.;.g1..│
---
libavcodec/h2645_parse.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
index 28db465059..9f66f079c2 100644
--- a/libavcodec/h2645_parse.c
+++ b/libavcodec/h2645_parse.c
@@ -40,8 +40,9 @@ int ff_h2645_extract_rbsp(const uint8_t *src, int length,
nal->skipped_bytes = 0;
#define STARTCODE_TEST \
-if (i + 2 < length && src[i + 1] == 0 && src[i + 2] <= 3) { \
-if (src[i + 2] != 3 && src[i + 2] != 0) { \
+if (i + 2 < length && src[i + 1] == 0 &&\
+ (src[i + 2] == 3 || src[i + 2] == 1)) { \
+if (src[i + 2] == 1) { \
/* startcode, so we must be past the end */ \
length = i; \
} \
--
2.43.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH v1] avcodec/h2645_parse: Don't treat 0x000002 as a start code and truncate
According to ITU-T H.265 7.4.2.1 this byte sequence should not appear in a
NAL unit but in practice in rare cases it seems it does, possibly due to buggy
encoders. Other players like VLC and Quicktime seem to be fine with it.
Currently when this sequence is found it is treated as if the next start code
has been found and the NAL unit gets truncated.
This change limits the code to only look for first start code 0x001 or
first escape 0x03.
Sadly i can't share the original source file with the issue but the first
80 bytes of the NAL unit looks like this:
│00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f│0123456789abcdef│
0x0│00 00 00 01 02 01 d0 bc 57 a1 b8 44 70 01 00 0b│W..Dp...│
0x00010│80 2e 00 c2 6c ec 3e b9 e3 03 fb 91 2e d2 43 cb│l.>...C.│
0x00020│1d 2c 00 00 02 00 02 00 5c 93 72 6f 31 76 18 00│.,..\.ro1v..│
0x00030│08 38 aa b1 4c 33 3f fd 08 cb 77 9b d4 3c db 02│.8..L3?...w..<..│
0x00040│a2 04 73 15 75 de 3b c4 67 c0 8f ca ad 31 f1 99│..s.u.;.g1..│
---
libavcodec/h2645_parse.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
index 28db465059..d54bdd53c2 100644
--- a/libavcodec/h2645_parse.c
+++ b/libavcodec/h2645_parse.c
@@ -40,8 +40,9 @@ int ff_h2645_extract_rbsp(const uint8_t *src, int length,
nal->skipped_bytes = 0;
#define STARTCODE_TEST \
-if (i + 2 < length && src[i + 1] == 0 && src[i + 2] <= 3) { \
-if (src[i + 2] != 3 && src[i + 2] != 0) { \
+ if (i + 2 < length && src[i + 1] == 0 && \
+ (src[i + 2] == 3 || src[i + 2] == 1)) { \
+if (src[i + 2] == 1) { \
/* startcode, so we must be past the end */ \
length = i; \
} \
--
2.43.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avcodec/flac_parser: fix triggered assert
On Tue, Sep 13, 2022 at 11:28 AM Paul B Mahol wrote: > On 9/8/22, Paul B Mahol wrote: > > Patch attached. > > > > Will apply soon. Curious where the 1 << 28 constant come from? ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avocdec/flac_parser: another fix
On Mon, Sep 5, 2022 at 8:16 PM Paul B Mahol wrote:
> Patch attached.
>
Thanks and can confirm that the patch produces the same samples as the flac
reference decoder for the original file in
https://trac.ffmpeg.org/ticket/9621 that I could not share.
But I'm not sure I follow how the patch works. I read it as we skip
checking CRC if the current frame or sample number is the expected next one?
+if ((fpc->last_fi.frame_or_sample_num + 1 ==
header_fi->frame_or_sample_num) ||
+(fpc->last_fi.frame_or_sample_num + fpc->last_fi.blocksize ==
header_fi->frame_or_sample_num)) {
Would it make sense to look at the blocking strategy bit to know which one
to check?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] x86/tx_float: add support for calling assembly functions from assembly
On Tue, Sep 6, 2022 at 3:45 PM Martin Storsjö wrote: > On Tue, 6 Sep 2022, Mattias Wadman wrote: > > > On Sat, Sep 3, 2022 at 3:41 AM Lynne wrote: > > > >> Needed for the next patch. > >> We get this for the extremely small cost of a branch on _ns functions, > >> which wouldn't be used anyway with assembly. > >> > >> Patch attached. > >> > > > > Hi, I have issues building on macOS (12.5.1) with this patch. Maybe I'm > > missing something? old nasm version? > > That patch needed manually adding the symbol prefix in a few places, for > platforms that do use symbol prefixes. See the patch that I just posted, > which should fix this. > > Great, thanks! ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] x86/tx_float: add support for calling assembly functions from assembly
On Sat, Sep 3, 2022 at 3:41 AM Lynne wrote: > Needed for the next patch. > We get this for the extremely small cost of a branch on _ns functions, > which wouldn't be used anyway with assembly. > > Patch attached. > Hi, I have issues building on macOS (12.5.1) with this patch. Maybe I'm missing something? old nasm version? $ make V=1 nasm -f macho64 -DPIC -DPREFIX -I./ -I.// -Ilibavutil/x86/ -Pconfig.asm -MD libavutil/x86/tx_float.d -o libavutil/x86/tx_float.o libavutil/x86/tx_float.asm libavutil/x86/tx_float.asm:753: error: symbol `ff_tx_fft8_asm_float_sse3' not defined libavutil/x86/tx_float.asm:747: ... from macro `FFT8_SSE_FN' defined here libavutil/x86/x86util.asm:1180: ... from macro `call' defined here libavutil/x86/x86util.asm:1192: ... from macro `call_internal' defined here libavutil/x86/tx_float.asm:793: error: symbol `ff_tx_fft8_asm_float_avx' not defined libavutil/x86/tx_float.asm:787: ... from macro `FFT8_AVX_FN' defined here libavutil/x86/x86util.asm:1180: ... from macro `call' defined here libavutil/x86/x86util.asm:1192: ... from macro `call_internal' defined here libavutil/x86/tx_float.asm:842: error: symbol `ff_tx_fft16_asm_float_avx' not defined libavutil/x86/tx_float.asm:836: ... from macro `FFT16_FN' defined here libavutil/x86/x86util.asm:1180: ... from macro `call' defined here libavutil/x86/x86util.asm:1192: ... from macro `call_internal' defined here libavutil/x86/tx_float.asm:844: error: symbol `ff_tx_fft16_asm_float_fma3' not defined libavutil/x86/tx_float.asm:836: ... from macro `FFT16_FN' defined here libavutil/x86/x86util.asm:1180: ... from macro `call' defined here libavutil/x86/x86util.asm:1192: ... from macro `call_internal' defined here libavutil/x86/tx_float.asm:927: error: symbol `ff_tx_fft32_asm_float_avx' not defined libavutil/x86/tx_float.asm:920: ... from macro `FFT32_FN' defined here libavutil/x86/x86util.asm:1180: ... from macro `call' defined here libavutil/x86/x86util.asm:1192: ... from macro `call_internal' defined here libavutil/x86/tx_float.asm:929: error: symbol `ff_tx_fft32_asm_float_fma3' not defined libavutil/x86/tx_float.asm:920: ... from macro `FFT32_FN' defined here libavutil/x86/x86util.asm:1180: ... from macro `call' defined here libavutil/x86/x86util.asm:1192: ... from macro `call_internal' defined here libavutil/x86/tx_float.asm:1352: error: symbol `ff_tx_fft_sr_asm_float_fma3' not defined libavutil/x86/tx_float.asm:1345: ... from macro `FFT_SPLIT_RADIX_FN' defined here libavutil/x86/x86util.asm:1180: ... from macro `call' defined here libavutil/x86/x86util.asm:1192: ... from macro `call_internal' defined here libavutil/x86/tx_float.asm:1355: error: symbol `ff_tx_fft_sr_asm_float_avx2' not defined libavutil/x86/tx_float.asm:1345: ... from macro `FFT_SPLIT_RADIX_FN' defined here libavutil/x86/x86util.asm:1180: ... from macro `call' defined here libavutil/x86/x86util.asm:1192: ... from macro `call_internal' defined here $ nasm --version NASM version 2.15.05 compiled on Nov 14 2020 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH] avfilter/vf_zscale: Add smpte240m transfer option and fix matrix option typo
---
libavfilter/vf_zscale.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/libavfilter/vf_zscale.c b/libavfilter/vf_zscale.c
index 91166dcbcb..999147b587 100644
--- a/libavfilter/vf_zscale.c
+++ b/libavfilter/vf_zscale.c
@@ -1033,6 +1033,7 @@ static const AVOption zscale_options[] = {
{ "bt470m", 0, 0, AV_OPT_TYPE_CONST,
{.i64 = ZIMG_TRANSFER_470_M}, 0, 0, FLAGS, "transfer" },
{ "bt470bg", 0, 0, AV_OPT_TYPE_CONST,
{.i64 = ZIMG_TRANSFER_470_BG}, 0, 0, FLAGS, "transfer" },
{ "smpte170m",0, 0, AV_OPT_TYPE_CONST,
{.i64 = ZIMG_TRANSFER_601}, 0, 0, FLAGS, "transfer" },
+{ "smpte240m",0, 0, AV_OPT_TYPE_CONST,
{.i64 = ZIMG_TRANSFER_240M},0, 0, FLAGS, "transfer" },
{ "bt709",0, 0, AV_OPT_TYPE_CONST,
{.i64 = ZIMG_TRANSFER_709}, 0, 0, FLAGS, "transfer" },
{ "linear", 0, 0, AV_OPT_TYPE_CONST,
{.i64 = ZIMG_TRANSFER_LINEAR}, 0, 0, FLAGS, "transfer" },
{ "log100", 0, 0, AV_OPT_TYPE_CONST,
{.i64 = ZIMG_TRANSFER_LOG_100}, 0, 0, FLAGS, "transfer" },
@@ -1058,7 +1059,7 @@ static const AVOption zscale_options[] = {
{ "fcc", 0, 0, AV_OPT_TYPE_CONST,
{.i64 = ZIMG_MATRIX_FCC}, 0, 0, FLAGS, "matrix" },
{ "bt470bg", 0, 0, AV_OPT_TYPE_CONST,
{.i64 = ZIMG_MATRIX_470BG}, 0, 0, FLAGS, "matrix" },
{ "smpte170m",0, 0, AV_OPT_TYPE_CONST,
{.i64 = ZIMG_MATRIX_170M},0, 0, FLAGS, "matrix" },
-{ "smpte2400m", 0, 0, AV_OPT_TYPE_CONST,
{.i64 = ZIMG_MATRIX_240M},0, 0, FLAGS, "matrix" },
+{ "smpte240m",0, 0, AV_OPT_TYPE_CONST,
{.i64 = ZIMG_MATRIX_240M},0, 0, FLAGS, "matrix" },
{ "ycgco",0, 0, AV_OPT_TYPE_CONST,
{.i64 = ZIMG_MATRIX_YCGCO}, 0, 0, FLAGS, "matrix" },
{ "bt2020nc", 0, 0, AV_OPT_TYPE_CONST,
{.i64 = ZIMG_MATRIX_2020_NCL},0, 0, FLAGS, "matrix" },
{ "bt2020c", 0, 0, AV_OPT_TYPE_CONST,
{.i64 = ZIMG_MATRIX_2020_CL}, 0, 0, FLAGS, "matrix" },
--
2.29.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avcodec/flac_parser: Consider AV_INPUT_BUFFER_PADDING_SIZE
On Thu, Oct 21, 2021 at 10:35 PM Michael Niedermayer
wrote:
> On Thu, Oct 21, 2021 at 10:17:25PM +0200, Paul B Mahol wrote:
> > LGTM for now
>
> will apply the improved variant below
>
> diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c
> index 2c550507fc8..3b27b152fc5 100644
> --- a/libavcodec/flac_parser.c
> +++ b/libavcodec/flac_parser.c
> @@ -55,6 +55,7 @@
>
> /** largest possible size of flac header */
> #define MAX_FRAME_HEADER_SIZE 16
> +#define MAX_FRAME_VERIFY_SIZE (MAX_FRAME_HEADER_SIZE + 1)
>
> typedef struct FLACHeaderMarker {
> int offset; /**< byte offset from start of
> FLACParseContext->buffer */
> @@ -99,7 +100,7 @@ static int frame_header_is_valid(AVCodecContext *avctx,
> const uint8_t *buf,
> uint8_t subframe_type;
>
> // header plus one byte from first subframe
> -init_get_bits(, buf, MAX_FRAME_HEADER_SIZE * 8 + 8);
> +init_get_bits(, buf, MAX_FRAME_VERIFY_SIZE * 8);
> if (ff_flac_decode_frame_header(avctx, , fi, 127)) {
> return 0;
> }
> @@ -196,7 +197,7 @@ static int
> find_headers_search_validate(FLACParseContext *fpc, int offset)
> uint8_t *header_buf;
> int size = 0;
> header_buf = flac_fifo_read_wrap(fpc, offset,
> - MAX_FRAME_HEADER_SIZE,
> + MAX_FRAME_VERIFY_SIZE +
> AV_INPUT_BUFFER_PADDING_SIZE,
> >wrap_buf,
> >wrap_buf_allocated_size);
> if (frame_header_is_valid(fpc->avctx, header_buf, )) {
>
>
LGTM
But i'm not sure about the PARSER_FLAG_COMPLETE_FRAMES case, hard to tell
if those code paths will always have
MAX_FRAME_VERIFY_SIZE+AV_INPUT_BUFFER_PADDING_SIZE buf size.
Thanks for helping to fix this.
BTW, yesterday a FLAC file showed up with a "false" frame that even this
patch failed to ignore. Strange enough it is a FLAC file with no encoder
metadata at all and the frame that it failed on is a verbatim frame. It's a
perfectly valid file with correct md5 but the audio is heavily distorted
which explains the verbatim frames. Hopefully they should be very rare.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v1] libavcodec/flac_parser: Validate subframe zero bit and type
On Wed, Oct 20, 2021 at 11:07 PM Michael Niedermayer wrote: > On Wed, Oct 13, 2021 at 06:15:26PM +0200, Mattias Wadman wrote: > > Reduces the risk of finding false frames that happens to have valid > values and CRC. > > > > Fixes ticket #9185 ffmpeg flac decoder incorrectly finds junk frame > > https://trac.ffmpeg.org/ticket/9185 > > --- > > libavcodec/flac_parser.c | 30 -- > > 1 file changed, 28 insertions(+), 2 deletions(-) > > After this was applied out of array accesses appeared > > tools/target_dem_flac_fuzzer: Running 1 inputs 1 time(s) each. > Running: > libfuzz-repro/40109/clusterfuzz-testcase-minimized-ffmpeg_dem_FLAC_fuzzer-4805686811295744 > = > ==30399==ERROR: AddressSanitizer: heap-buffer-overflow on address > 0x63352800 at pc 0x01734048 bp 0x7ffd3598d090 sp 0x7ffd3598d088 > READ of size 4 at 0x63352800 thread T0 > #0 0x1734047 in get_bits libavcodec/get_bits.h:404:5 > #1 0x1734047 in frame_header_is_valid libavcodec/flac_parser.c:118 > #2 0x173ac77 in find_headers_search_validate > libavcodec/flac_parser.c:202:9 > #3 0x173a851 in find_headers_search libavcodec/flac_parser.c:248:31 > #4 0x172f29b in find_new_headers libavcodec/flac_parser.c:268:18 > #5 0x172f29b in flac_parse libavcodec/flac_parser.c:666 > #6 0x13fb138 in av_parser_parse2 libavcodec/parser.c:165:13 > #7 0x6f1dba in parse_packet libavformat/demux.c:1127:15 > #8 0x6befe7 in read_frame_internal libavformat/demux.c:1322:24 > #9 0x6bafca in av_read_frame libavformat/demux.c:1426:17 > #10 0x4c7832 in LLVMFuzzerTestOneInput tools/target_dem_fuzzer.c:197:15 > #11 0x27055bd in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, > unsigned long) Fuzzer/build/../FuzzerLoop.cpp:495:13 > #12 0x26fa192 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, > unsigned long) Fuzzer/build/../FuzzerDriver.cpp:273:6 > #13 0x26ff391 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned > char const*, unsigned long)) Fuzzer/build/../FuzzerDriver.cpp:690:9 > #14 0x26f9e70 in main Fuzzer/build/../FuzzerMain.cpp:20:10 > #15 0x7f5d338b8bf6 in __libc_start_main > /build/glibc-S9d2JN/glibc-2.27/csu/../csu/libc-start.c:310 > #16 0x41fb79 in _start (tools/target_dem_flac_fuzzer+0x41fb79) > > 0x63352800 is located 0 bytes to the right of 106496-byte region > [0x63338800,0x63352800) > allocated by thread T0 here: > #0 0x498597 in posix_memalign > /b/swarming/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_malloc_linux.cc:226:3 > #1 0x26112c8 in av_malloc libavutil/mem.c:104:9 > #2 0x25a67b3 in av_fifo_alloc_array libavutil/fifo.c:51:20 > #3 0x172c9b6 in flac_parse_init libavcodec/flac_parser.c:747:21 > #4 0x13f7d67 in av_parser_init libavcodec/parser.c:67:15 > #5 0x6ce0b8 in avformat_find_stream_info libavformat/demux.c:2453:27 > #6 0x4c77d0 in LLVMFuzzerTestOneInput tools/target_dem_fuzzer.c:192:11 > #7 0x27055bd in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, > unsigned long) Fuzzer/build/../FuzzerLoop.cpp:495:13 > #8 0x26fa192 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, > unsigned long) Fuzzer/build/../FuzzerDriver.cpp:273:6 > #9 0x26ff391 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned > char const*, unsigned long)) Fuzzer/build/../FuzzerDriver.cpp:690:9 > #10 0x26f9e70 in main Fuzzer/build/../FuzzerMain.cpp:20:10 > #11 0x7f5d338b8bf6 in __libc_start_main > /build/glibc-S9d2JN/glibc-2.27/csu/../csu/libc-start.c:310 > > SUMMARY: AddressSanitizer: heap-buffer-overflow > libavcodec/get_bits.h:404:5 in get_bits > Shadow bytes around the buggy address: > 0x0c66800024b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x0c66800024c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x0c66800024d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x0c66800024e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x0c66800024f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > =>0x0c6680002500:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > 0x0c6680002510: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > 0x0c6680002520: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > 0x0c6680002530: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > 0x0c6680002540: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > 0x0c6680002550: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > Shadow byte legend (one shadow byte represents 8 application bytes): > Addressable: 00 > Partially addressable: 01 02 03 04 05 06 07 > Heap left redzone: fa > Freed heap region: fd > Stack left
Re: [FFmpeg-devel] [PATCH v1] libavcodec/flac_parser: Validate subframe zero bit and type
Thanks for taking your time to review On Mon, Oct 18, 2021 at 5:16 PM Paul B Mahol wrote: > LGTM, will apply soon > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v1] libavcodec/flac_parser: Validate subframe zero bit and type
On Wed, Oct 13, 2021 at 6:15 PM Mattias Wadman
wrote:
> Reduces the risk of finding false frames that happens to have valid values
> and CRC.
>
> Fixes ticket #9185 ffmpeg flac decoder incorrectly finds junk frame
> https://trac.ffmpeg.org/ticket/9185
> ---
> libavcodec/flac_parser.c | 30 --
> 1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c
> index d3d9c889a1..2c550507fc 100644
> --- a/libavcodec/flac_parser.c
> +++ b/libavcodec/flac_parser.c
> @@ -96,8 +96,34 @@ static int frame_header_is_valid(AVCodecContext *avctx,
> const uint8_t *buf,
> FLACFrameInfo *fi)
> {
> GetBitContext gb;
> -init_get_bits(, buf, MAX_FRAME_HEADER_SIZE * 8);
> -return !ff_flac_decode_frame_header(avctx, , fi, 127);
> +uint8_t subframe_type;
> +
> +// header plus one byte from first subframe
> +init_get_bits(, buf, MAX_FRAME_HEADER_SIZE * 8 + 8);
> +if (ff_flac_decode_frame_header(avctx, , fi, 127)) {
> +return 0;
> +}
> +// subframe zero bit
> +if (get_bits1() != 0) {
> +return 0;
> +}
> +// subframe type
> +// 00 : SUBFRAME_CONSTANT
> +// 01 : SUBFRAME_VERBATIM
> +// 1x : reserved
> +// 0001xx : reserved
> +// 001xxx : if(xxx <= 4) SUBFRAME_FIXED, xxx=order ; else reserved
> +// 01 : reserved
> +// 1x : SUBFRAME_LPC, x=order-1
> +subframe_type = get_bits(, 6);
> +if (!(subframe_type == 0 ||
> + subframe_type == 1 ||
> + ((subframe_type >= 8) && (subframe_type <= 12)) ||
> + (subframe_type >= 32))) {
> +return 0;
> +}
> +
> +return 1;
> }
>
> /**
> --
> 2.29.2
>
>
Ping
Maybe I should give some more context. I originally reported
https://trac.ffmpeg.org/ticket/9185 after seeing decode errors 1-2 times a
day in a system that decodes tens of thousands of FLAC files per day. All
of the failing ones I've looked at decodes fine with the FLAC reference
decoder and nearly all of them are encoded using "Switch Audio File
Converter", but I've managed to make ffmpeg's FLAC encoder produce valid
files that it fails on also.
With the patch I haven't seen any errors for the last 2 weeks and I've also
gone through older files and all have decoded fine.
I've looked into solving this without adding more validation in the flac
parser but failed. I can't see how to know the frame size without more or
less decoding it.
-Mattias
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH v1] libavcodec/flac_parser: Validate subframe zero bit and type
Reduces the risk of finding false frames that happens to have valid values and
CRC.
Fixes ticket #9185 ffmpeg flac decoder incorrectly finds junk frame
https://trac.ffmpeg.org/ticket/9185
---
libavcodec/flac_parser.c | 30 --
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c
index d3d9c889a1..2c550507fc 100644
--- a/libavcodec/flac_parser.c
+++ b/libavcodec/flac_parser.c
@@ -96,8 +96,34 @@ static int frame_header_is_valid(AVCodecContext *avctx,
const uint8_t *buf,
FLACFrameInfo *fi)
{
GetBitContext gb;
-init_get_bits(, buf, MAX_FRAME_HEADER_SIZE * 8);
-return !ff_flac_decode_frame_header(avctx, , fi, 127);
+uint8_t subframe_type;
+
+// header plus one byte from first subframe
+init_get_bits(, buf, MAX_FRAME_HEADER_SIZE * 8 + 8);
+if (ff_flac_decode_frame_header(avctx, , fi, 127)) {
+return 0;
+}
+// subframe zero bit
+if (get_bits1() != 0) {
+return 0;
+}
+// subframe type
+// 00 : SUBFRAME_CONSTANT
+// 01 : SUBFRAME_VERBATIM
+// 1x : reserved
+// 0001xx : reserved
+// 001xxx : if(xxx <= 4) SUBFRAME_FIXED, xxx=order ; else reserved
+// 01 : reserved
+// 1x : SUBFRAME_LPC, x=order-1
+subframe_type = get_bits(, 6);
+if (!(subframe_type == 0 ||
+ subframe_type == 1 ||
+ ((subframe_type >= 8) && (subframe_type <= 12)) ||
+ (subframe_type >= 32))) {
+return 0;
+}
+
+return 1;
}
/**
--
2.29.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] fftools/ffmpeg: Add new variant source_no_drop to the force_key_frames option
On Fri, Jul 16, 2021 at 9:51 AM Thilo Borgmann wrote: > Am 04.07.21 um 21:23 schrieb Michael Niedermayer: > > On Sun, Jun 20, 2021 at 09:48:29PM +0200, Thilo Borgmann wrote: > >> Hi, > >> > >> adds a new variant to the -force_key_frames option "source_no_drop" to > make > >> sure whenever a key frame in the source is dropped, we choose the next > frame > >> in the output stream as a key frame. > >> > >> -Thilo > > > >> doc/ffmpeg.texi |7 +++ > >> fftools/ffmpeg.c |6 ++ > >> fftools/ffmpeg.h |1 + > >> 3 files changed, 14 insertions(+) > >> e8ffd04204a6cc243b2f98858ec1f1afe44ff8e8 > 0001-fftools-ffmpeg-Add-new-variant-source_no_drop-to-the.patch > >> From b37c854e7d48b538537b2c9c5e9651ba2ead3e52 Mon Sep 17 00:00:00 2001 > >> From: Keyun Tong > >> Date: Sun, 20 Jun 2021 21:42:29 +0200 > >> Subject: [PATCH] fftools/ffmpeg: Add new variant source_no_drop to the > >> force_key_frames option > > > > LGTM if both variants have a use case > > Pushed. Sorry for late review, i just noticed this line: "!strncmp(ost->forced_keyframes, "source_no_drop", 6)" shouldn't it be: !strncmp(ost->forced_keyframes, "source_no_drop", 14) ? ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] Project orientation
On Mon, Jul 6, 2020 at 5:59 PM Olly Woodman wrote: > > On Mon, 6 Jul 2020 at 05:54, Jim DeLaHunt wrote: > > > On 2020-07-04 07:43, Nicolas George wrote: > > > [In the FFmpeg project,] [t]here is work in making highly-optimized > > > decoders, this work is impressive and creative…. But as far as I can see, > > > that is mostly all there is going on. The rest seems to be rather basic: > > > fixing bugs…, implementing support for > > > features that were decided and designed elsewhere. > > > > > > And it is not just that it is not happening: there is genuine opposition > > > to creativity: things that are not already justified externally, things > > > that do not look like well-known patterns, are met with scepticism and > > > eventually rejected. > > > > > > At this point, we should ask ourselves: > > > > > > Is it what we want the project to be, or is it just what we have let it > > > become? > > > > > > I do not think this evolution is the result of a deliberate choice. I > > > think it is more the result of the stress of success and the stress of a > > > fork. Success can stifle creativity, because creativity implies the risk > > > of failure: the project has become advert to risk. > > > > > > It has evolved that way, but are we fine with it? > > > > > > I personally am not…. > > > > I am really happy to see this discussion starting up. It is good timing; > > some new governance committees are forming. They might be able to take > > it on. From my perspective as an outside observer, I think the project > > will benefit from you — the people who are at the heart of the project — > > having this discussion. > > > > For what purpose do all of you contribute to FFmpeg? What is the good > > you see FFmpeg doing? What is FFmpeg's purpose? What stands in the way > > of FFmpeg achieving its purpose, and of you achieving your own > > individual purpose? > > > > For me, several comments in the thread resonated. They have a theme: > > bringing in new participants: > > > > On 2020-07-04 08:37, James Almer wrote: > > > …Another thing worth mentioning is a lack of new blood. Despite > > > participating in GSoC for a long while, i can't name a student that > > > stuck around after the fact. Mind, there are new devs that started > > > contributing for other reasons, but perhaps not enough? > > > > > > On 2020-07-04 11:20, Soft Works wrote: > > > > > …As a developer (without a well-known name) who wants to contribute a > > patch, > > > things can be quite frustrating here. When that patch accidentally hits > > an > > > area of one the very few who are caring and friendly - you're lucky. > > > But otherwise, a patch will either be ignored or talked into infinity. > > > > > > I have a number of things to contribute, but after it didn’t work well > > > with small things, I decided not to bother with the bigger ones. > > > > On 2020-07-05 11:42, Steinar H. Gunderson wrote: > > > On Sun, Jul 05, 2020 at 08:13:25PM +0200, Manolis Stamatogiannakis wrote: > > >> As a fresh contributor, setting up git send-email was a hassle, but > > >> not an insurmountable obstacle. > > > Speaking only for myself, having sent a single-digit number of patches > > > to FFmpeg ever: Setting up git send-email was not a big turnoff. Having > > your > > > patch being not responded to (whether being forgotten, not found > > interesting > > > enough, or whatever other reason) was. > > > > > > To the extent you decide that you want new participants to join you on > > FFmpeg, I'm sorry to say, I think you have a problem. My own experience > > is that this project is more hostile to newcomers than many other free > > culture projects I know (e.g. Python, Thunderbird, Gnucash, Drupal, > > MusicBrainz, Wikipedia). It's not just your rudeness to each other on > > the email lists. It's not just the inattention to patches from new > > developers. It's also a simple lack of telling newcomers: welcome, we > > are glad you are here, we want your help, we will help you learn. > > > > As someone who recently tried to send their first patch to FFmpeg, > I'd also like to +1 this. It was completely ignored, despite sending > three reminders (sending a reminder is suggested in the FFmpeg > documentation for submitting patches). Perhaps it was ignored for > a good reason, but a non-response does not tell me that, and nor > does it encourage further involvement. > > I've also been quite shocked by some of the rudeness on this mailing > list, and the level to which it appears to be tolerated (or at least, the > extent to which there appear to be no repercussions for those who > repeatedly behave in a way that I'd consider unacceptable in a > professional context). +1 on this. Personally i didn't feel ignored and got good code review feedback. But the amount of rudeness I see on this list i think discourages people from participating. Also think gitlab or something similar would make things easier. > > > > Even more deeply, the culture of this project is focussed on
Re: [FFmpeg-devel] [PATCH v5] libavformat/flacdec: Workaround for truncated metadata picture size
Hello, ok to merge?
On Tue, May 19, 2020 at 11:27 AM Mattias Wadman
wrote:
>
> Some flac muxers write truncated metadata picture size if the picture
> data do not fit in 24 bits. Detect this by truncting the size found inside
> the picture block and if it matches the block size use it and read rest
> of picture data.
>
> This workaround is only for flac files and not ogg files with flac
> METADATA_BLOCK_PICTURE comments and it can be disabled with strict level
> above normal. Currently there is a 500MB limit on truncate size to protect
> from large memory allocations.
>
> The truncation bug in lavf flacenc was fixed in
> e447a4d112bcfee10126c54eb4481fa8712957c8
> but based on existing broken files other unknown flac muxers seems to
> truncate also.
> Before the fix a broken flac file for reproduction could be generated with:
> ffmpeg -f lavfi -i sine -f lavfi -i color=red:size=2400x2400 -map 0:0 -map
> 1:0 -c:v:0 bmp -disposition:1 attached_pic -t 1 test.flac
>
> Fixes ticket 6333
> ---
> libavformat/flac_picture.c | 47 ++--
> libavformat/flac_picture.h | 2 +-
> libavformat/flacdec.c| 2 +-
> libavformat/oggparsevorbis.c | 2 +-
> 4 files changed, 42 insertions(+), 11 deletions(-)
>
> diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
> index 81ddf80465..53e24b28b7 100644
> --- a/libavformat/flac_picture.c
> +++ b/libavformat/flac_picture.c
> @@ -27,7 +27,9 @@
> #include "id3v2.h"
> #include "internal.h"
>
> -int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
> +#define MAX_TRUNC_PICTURE_SIZE (500 * 1024 * 1024)
> +
> +int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size,
> int truncate_workaround)
> {
> const CodecMime *mime = ff_id3v2_mime_tags;
> enum AVCodecID id = AV_CODEC_ID_NONE;
> @@ -36,7 +38,8 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf,
> int buf_size)
> GetByteContext g;
> AVStream *st;
> int width, height, ret = 0;
> -unsigned int len, type;
> +unsigned int type;
> +uint32_t len, left, trunclen = 0;
>
> if (buf_size < 34) {
> av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> short\n");
> @@ -114,16 +117,44 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t
> *buf, int buf_size)
>
> /* picture data */
> len = bytestream2_get_be32u();
> -if (len <= 0 || len > bytestream2_get_bytes_left()) {
> -av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> short\n");
> -if (s->error_recognition & AV_EF_EXPLODE)
> -ret = AVERROR_INVALIDDATA;
> -goto fail;
> +
> +left = bytestream2_get_bytes_left();
> +if (len <= 0 || len > left) {
> +if (len > MAX_TRUNC_PICTURE_SIZE || len >= INT_MAX -
> AV_INPUT_BUFFER_PADDING_SIZE) {
> +av_log(s, AV_LOG_ERROR, "Attached picture metadata block too big
> %u\n", len);
> +if (s->error_recognition & AV_EF_EXPLODE)
> +ret = AVERROR_INVALIDDATA;
> +goto fail;
> +}
> +
> +// Workaround bug for flac muxers that writs truncated metadata
> picture block size if
> +// the picture size do not fit in 24 bits. lavf flacenc used to have
> the issue and based
> +// on existing broken files other unknown flac muxers seems to
> truncate also.
> +if (truncate_workaround &&
> +s->strict_std_compliance <= FF_COMPLIANCE_NORMAL &&
> +len > left && (len & 0xff) == left) {
> +av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture
> size from %u to %u\n", left, len);
> +trunclen = len - left;
> +} else {
> +av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> short\n");
> +if (s->error_recognition & AV_EF_EXPLODE)
> +ret = AVERROR_INVALIDDATA;
> +goto fail;
> +}
> }
> if (!(data = av_buffer_alloc(len + AV_INPUT_BUFFER_PADDING_SIZE))) {
> RETURN_ERROR(AVERROR(ENOMEM));
> }
> -bytestream2_get_bufferu(, data->data, len);
> +
> +if (trunclen == 0) {
> +bytestream2_get_bufferu(, data->data, len);
> +} else {
> +// If truncation was detected copy all data from block and read
> missing bytes
> +// not included in the block size
> +bytestream2_get_bufferu(, data->data, left);
> +if (avio_read(s->pb, data->data + len - trunclen,
Re: [FFmpeg-devel] [PATCH v4 1/2] fftools: add options to dump filter graph
On Sat, May 23, 2020 at 2:23 AM wrote:
>
> From: Limin Wang
>
> Signed-off-by: Limin Wang
> ---
> doc/ffmpeg.texi | 15
> fftools/ffmpeg.h| 2 ++
> fftools/ffmpeg_filter.c | 20
> fftools/ffmpeg_opt.c| 20
> libavfilter/graphdump.c | 63
> +
> 5 files changed, 120 insertions(+)
>
> diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi
> index ed437bb..61f 100644
> --- a/doc/ffmpeg.texi
> +++ b/doc/ffmpeg.texi
> @@ -735,6 +735,21 @@ Technical note -- attachments are implemented as codec
> extradata, so this
> option can actually be used to extract extradata from any stream, not just
> attachments.
>
> +@item -dump_filtergraph @var{filename} (@emph{global})
> +Set the output file name of filter graph for dump.
> +
> +It is "ASCII" format by default. for Graphviz DOT output format,
> +you can convert it to png by GraphViz tool:
> +@example
> +dot -Tpng dump_fg_filename -o dump_graph.png
> +@end example
> +
> +@item -dump_filtergraph_format @var{format} (@emph{global})
> +Set the output format of filter graph for dump. Support format: "DOT",
> "ASCII".
> +
> +DOT is the text file format of the suite GraphViz, ASCII is the text file
> format
> +of ASCII style.
> +
> @item -noautorotate
> Disable automatically rotating video based on file metadata.
>
> diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
> index 38205a1..55f115b 100644
> --- a/fftools/ffmpeg.h
> +++ b/fftools/ffmpeg.h
> @@ -606,6 +606,8 @@ extern AVIOContext *progress_avio;
> extern float max_error_rate;
> extern char *videotoolbox_pixfmt;
>
> +extern char* dump_fg_filename;
> +extern char* dump_fg_format;
> extern int filter_nbthreads;
> extern int filter_complex_nbthreads;
> extern int vstats_version;
> diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c
> index 8b5b157..485fc73 100644
> --- a/fftools/ffmpeg_filter.c
> +++ b/fftools/ffmpeg_filter.c
> @@ -1106,6 +1106,26 @@ int configure_filtergraph(FilterGraph *fg)
> if ((ret = avfilter_graph_config(fg->graph, NULL)) < 0)
> goto fail;
>
> +if (dump_fg_filename) {
> +char *dump = avfilter_graph_dump(fg->graph, dump_fg_format);
> +FILE *fg_file = fopen(dump_fg_filename, "w");
> +
> +if (!dump) {
> +ret = AVERROR(ENOMEM);
> +goto fail;
> +}
> +if (fg_file) {
> +fputs(dump, fg_file);
> +fflush(fg_file);
> +fclose(fg_file);
> +} else {
> +ret = AVERROR(EINVAL);
> +av_free(dump);
> +goto fail;
> +}
> +av_free(dump);
> +}
> +
> /* limit the lists of allowed formats to the ones selected, to
> * make sure they stay the same if the filtergraph is reconfigured later
> */
> for (i = 0; i < fg->nb_outputs; i++) {
> diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
> index 60bb437..bdd8957 100644
> --- a/fftools/ffmpeg_opt.c
> +++ b/fftools/ffmpeg_opt.c
> @@ -143,6 +143,8 @@ HWDevice *filter_hw_device;
>
> char *vstats_filename;
> char *sdp_filename;
> +char *dump_fg_filename;
> +char *dump_fg_format;
>
> float audio_drift_threshold = 0.1;
> float dts_delta_threshold = 10;
> @@ -2930,6 +2932,20 @@ static int opt_vstats(void *optctx, const char *opt,
> const char *arg)
> return opt_vstats_file(NULL, opt, filename);
> }
>
> +static int opt_fg_filename(void *optctx, const char *opt, const char *arg)
> +{
> +av_free (dump_fg_filename);
> +dump_fg_filename = av_strdup (arg);
> +return 0;
> +}
> +
> +static int opt_fg_format(void *optctx, const char *opt, const char *arg)
> +{
> +av_free (dump_fg_format);
> +dump_fg_format = av_strdup (arg);
> +return 0;
> +}
> +
> static int opt_video_frames(void *optctx, const char *opt, const char *arg)
> {
> OptionsContext *o = optctx;
> @@ -3548,6 +3564,10 @@ const OptionDef options[] = {
> { "dump_attachment", HAS_ARG | OPT_STRING | OPT_SPEC |
> OPT_EXPERT | OPT_INPUT, { .off
> = OFFSET(dump_attachment) },
> "extract an attachment into a file", "filename" },
> +{ "dump_filtergraph",HAS_ARG | OPT_EXPERT, {
> .func_arg = opt_fg_filename },
> +"the output filename of filter graph for dump", "filename" },
> +{ "dump_filtergraph_format", HAS_ARG | OPT_EXPERT, {
> .func_arg = opt_fg_format },
> +"the format of filter graph for dump, support DOT or ASCII"},
Possible to use AV_OPT_TYPE_CONST for these? maybe lowercase names?
Output for dot graph will very useful for me, thanks!
> { "stream_loop", OPT_INT | HAS_ARG | OPT_EXPERT | OPT_INPUT |
> OPT_OFFSET, { .off
> = OFFSET(loop) }, "set number of times input stream shall be looped", "loop
> count" },
> { "debug_ts", OPT_BOOL | OPT_EXPERT,
[FFmpeg-devel] [PATCH v5] libavformat/flacdec: Workaround for truncated metadata picture size
Some flac muxers write truncated metadata picture size if the picture
data do not fit in 24 bits. Detect this by truncting the size found inside
the picture block and if it matches the block size use it and read rest
of picture data.
This workaround is only for flac files and not ogg files with flac
METADATA_BLOCK_PICTURE comments and it can be disabled with strict level
above normal. Currently there is a 500MB limit on truncate size to protect
from large memory allocations.
The truncation bug in lavf flacenc was fixed in
e447a4d112bcfee10126c54eb4481fa8712957c8
but based on existing broken files other unknown flac muxers seems to truncate
also.
Before the fix a broken flac file for reproduction could be generated with:
ffmpeg -f lavfi -i sine -f lavfi -i color=red:size=2400x2400 -map 0:0 -map 1:0
-c:v:0 bmp -disposition:1 attached_pic -t 1 test.flac
Fixes ticket 6333
---
libavformat/flac_picture.c | 47 ++--
libavformat/flac_picture.h | 2 +-
libavformat/flacdec.c| 2 +-
libavformat/oggparsevorbis.c | 2 +-
4 files changed, 42 insertions(+), 11 deletions(-)
diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
index 81ddf80465..53e24b28b7 100644
--- a/libavformat/flac_picture.c
+++ b/libavformat/flac_picture.c
@@ -27,7 +27,9 @@
#include "id3v2.h"
#include "internal.h"
-int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
+#define MAX_TRUNC_PICTURE_SIZE (500 * 1024 * 1024)
+
+int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size, int
truncate_workaround)
{
const CodecMime *mime = ff_id3v2_mime_tags;
enum AVCodecID id = AV_CODEC_ID_NONE;
@@ -36,7 +38,8 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf,
int buf_size)
GetByteContext g;
AVStream *st;
int width, height, ret = 0;
-unsigned int len, type;
+unsigned int type;
+uint32_t len, left, trunclen = 0;
if (buf_size < 34) {
av_log(s, AV_LOG_ERROR, "Attached picture metadata block too short\n");
@@ -114,16 +117,44 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t
*buf, int buf_size)
/* picture data */
len = bytestream2_get_be32u();
-if (len <= 0 || len > bytestream2_get_bytes_left()) {
-av_log(s, AV_LOG_ERROR, "Attached picture metadata block too short\n");
-if (s->error_recognition & AV_EF_EXPLODE)
-ret = AVERROR_INVALIDDATA;
-goto fail;
+
+left = bytestream2_get_bytes_left();
+if (len <= 0 || len > left) {
+if (len > MAX_TRUNC_PICTURE_SIZE || len >= INT_MAX -
AV_INPUT_BUFFER_PADDING_SIZE) {
+av_log(s, AV_LOG_ERROR, "Attached picture metadata block too big
%u\n", len);
+if (s->error_recognition & AV_EF_EXPLODE)
+ret = AVERROR_INVALIDDATA;
+goto fail;
+}
+
+// Workaround bug for flac muxers that writs truncated metadata
picture block size if
+// the picture size do not fit in 24 bits. lavf flacenc used to have
the issue and based
+// on existing broken files other unknown flac muxers seems to
truncate also.
+if (truncate_workaround &&
+s->strict_std_compliance <= FF_COMPLIANCE_NORMAL &&
+len > left && (len & 0xff) == left) {
+av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture size
from %u to %u\n", left, len);
+trunclen = len - left;
+} else {
+av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
short\n");
+if (s->error_recognition & AV_EF_EXPLODE)
+ret = AVERROR_INVALIDDATA;
+goto fail;
+}
}
if (!(data = av_buffer_alloc(len + AV_INPUT_BUFFER_PADDING_SIZE))) {
RETURN_ERROR(AVERROR(ENOMEM));
}
-bytestream2_get_bufferu(, data->data, len);
+
+if (trunclen == 0) {
+bytestream2_get_bufferu(, data->data, len);
+} else {
+// If truncation was detected copy all data from block and read
missing bytes
+// not included in the block size
+bytestream2_get_bufferu(, data->data, left);
+if (avio_read(s->pb, data->data + len - trunclen, trunclen) < trunclen)
+RETURN_ERROR(AVERROR_INVALIDDATA);
+}
memset(data->data + len, 0, AV_INPUT_BUFFER_PADDING_SIZE);
if (AV_RB64(data->data) == PNGSIG)
diff --git a/libavformat/flac_picture.h b/libavformat/flac_picture.h
index 4374b6f4f6..61fd0c8806 100644
--- a/libavformat/flac_picture.h
+++ b/libavformat/flac_picture.h
@@ -26,6 +26,6 @@
#define RETURN_ERROR(code) do { ret = (code); goto fail; } while (0)
-int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size);
+int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size, int
truncate_workaround);
#endif /* AVFORMAT_FLAC_PICTURE_H */
diff --git a/libavformat/flacdec.c b/libavformat/flacdec.c
index cb516fb1f3..79c05f14bf 100644
--- a/libavformat/flacdec.c
Re: [FFmpeg-devel] [PATCH v3] libavformat/flacdec: Workaround for truncated metadata picture size
On Sat, May 16, 2020 at 9:59 PM Andreas Rheinhardt
wrote:
>
> Mattias Wadman:
> > lavf flacenc could previously write truncated metadata picture size if
> > the picture data did not fit in 24 bits. Detect this by truncting the
> > size found inside the picture block and if it matches the block size
> > use it and read rest of picture data.
> >
> > Also only enable this workaround flac files and not ogg files with flac
> > METADATA_BLOCK_PICTURE comment.
> >
> > flacenc was fixed in e447a4d112bcfee10126c54eb4481fa8712957c8
> > before the fix a broken flac for reproduction could be generated with:
> > ffmpeg -f lavfi -i sine -f lavfi -i color=red:size=2400x2400 -map 0:0 -map
> > 1:0 -c:v:0 bmp -disposition:1 attached_pic -t 1 test.flac
> >
> > Fixes ticket 6333
> > ---
> > libavformat/flac_picture.c | 35 +++
> > libavformat/flac_picture.h | 2 +-
> > libavformat/flacdec.c| 2 +-
> > libavformat/oggparsevorbis.c | 2 +-
> > 4 files changed, 30 insertions(+), 11 deletions(-)
> >
> > diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
> > index 81ddf80465..61277e9dee 100644
> > --- a/libavformat/flac_picture.c
> > +++ b/libavformat/flac_picture.c
> > @@ -27,7 +27,7 @@
> > #include "id3v2.h"
> > #include "internal.h"
> >
> > -int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
> > +int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size,
> > int truncate_workaround)
> > {
> > const CodecMime *mime = ff_id3v2_mime_tags;
> > enum AVCodecID id = AV_CODEC_ID_NONE;
> > @@ -36,7 +36,8 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t
> > *buf, int buf_size)
> > GetByteContext g;
> > AVStream *st;
> > int width, height, ret = 0;
> > -unsigned int len, type;
> > +unsigned int type;
> > +uint32_t len, left, trunclen = 0;
> >
> > if (buf_size < 34) {
> > av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> > short\n");
> > @@ -114,16 +115,34 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t
> > *buf, int buf_size)
> >
> > /* picture data */
> > len = bytestream2_get_be32u();
> > -if (len <= 0 || len > bytestream2_get_bytes_left()) {
> > -av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> > short\n");
> > -if (s->error_recognition & AV_EF_EXPLODE)
> > -ret = AVERROR_INVALIDDATA;
> > -goto fail;
> > +
> > +left = bytestream2_get_bytes_left();
> > +if (len <= 0 || len > left) {
> > +// Workaround lavf flacenc bug that allowed writing truncated
> > metadata picture block size if
>
> I have not found the typical lavf metadata in the file from #6333, so it
> seems we are not the only tool that created such out-of-spec files. This
> should be reflected in the comment.
Good point, have been clarified.
>
> > +// picture size did not fit in 24 bits
> > +if (truncate_workaround && len > left && (len & 0xff) == left)
> > {
>
> There should be a way to disable this workaround; after all, there'd be
> no remedy if the 32bit len field is bogus. Maybe check
> strict_std_compliance for being >= FF_COMPLIANCE_STRICT.
Fixed. Made it fail if strict level it above normal.
>
> > +av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture
> > size from %d to %d\n", left, len);
> > +trunclen = len - left;
> > +} else {
> > +av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> > short\n");
> > +if (s->error_recognition & AV_EF_EXPLODE)
> > +ret = AVERROR_INVALIDDATA;
> > +goto fail;
> > +}
> > }
> > if (!(data = av_buffer_alloc(len + AV_INPUT_BUFFER_PADDING_SIZE))) {
>
> av_buffer_alloc() takes an int as its size argument. len +
> AV_INPUT_BUFFER_PADDING_SIZE can be anything here; it can be positive or
> negative. It can even be wrapped around. You need to check len for being
> < INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE. I wonder whether it should not
> be restricted even more (otherwise it would be easy to force an
> allocation of close to 2GiB).
Fixed. Fails if >500MB or INT_MAX overflow.
>
> > RETURN_ERROR(AVERROR(ENOMEM));
> > }
> > -bytestream2_get_bufferu(, dat
[FFmpeg-devel] [PATCH v4] libavformat/flacdec: Workaround for truncated metadata picture size
Some flac muxers write truncated metadata picture size if the picture
data do not fit in 24 bits. Detect this by truncting the size found inside
the picture block and if it matches the block size use it and read rest
of picture data.
Used to be an issue with lavf flacenc that was fixed in
e447a4d112bcfee10126c54eb4481fa8712957c8
but based on exiting broken files other unknown flac muxers seems to do it also.
Before the fix a broken flac for reproduction could be generated with:
ffmpeg -f lavfi -i sine -f lavfi -i color=red:size=2400x2400 -map 0:0 -map 1:0
-c:v:0 bmp -disposition:1 attached_pic -t 1 test.flac
Also only enable this workaround flac files and not ogg files with flac
METADATA_BLOCK_PICTURE comment.
Can be disabled with strict level above normal and correctly there is a 500MB
limit
on truncate size to protect from large memory allocations.
Fixes ticket 6333
---
libavformat/flac_picture.c | 47 ++--
libavformat/flac_picture.h | 2 +-
libavformat/flacdec.c| 2 +-
libavformat/oggparsevorbis.c | 2 +-
4 files changed, 42 insertions(+), 11 deletions(-)
diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
index 81ddf80465..349f2067aa 100644
--- a/libavformat/flac_picture.c
+++ b/libavformat/flac_picture.c
@@ -27,7 +27,9 @@
#include "id3v2.h"
#include "internal.h"
-int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
+#define MAX_TRUNC_PICTURE_SIZE (500 * 1024 * 1024)
+
+int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size, int
truncate_workaround)
{
const CodecMime *mime = ff_id3v2_mime_tags;
enum AVCodecID id = AV_CODEC_ID_NONE;
@@ -36,7 +38,8 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf,
int buf_size)
GetByteContext g;
AVStream *st;
int width, height, ret = 0;
-unsigned int len, type;
+unsigned int type;
+uint32_t len, left, trunclen = 0;
if (buf_size < 34) {
av_log(s, AV_LOG_ERROR, "Attached picture metadata block too short\n");
@@ -114,16 +117,44 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t
*buf, int buf_size)
/* picture data */
len = bytestream2_get_be32u();
-if (len <= 0 || len > bytestream2_get_bytes_left()) {
-av_log(s, AV_LOG_ERROR, "Attached picture metadata block too short\n");
-if (s->error_recognition & AV_EF_EXPLODE)
-ret = AVERROR_INVALIDDATA;
-goto fail;
+
+left = bytestream2_get_bytes_left();
+if (len <= 0 || len > left) {
+if (len > MAX_TRUNC_PICTURE_SIZE || len >= INT_MAX -
AV_INPUT_BUFFER_PADDING_SIZE) {
+av_log(s, AV_LOG_ERROR, "Attached picture metadata block too big
%u\n", len);
+if (s->error_recognition & AV_EF_EXPLODE)
+ret = AVERROR_INVALIDDATA;
+goto fail;
+}
+
+// Workaround bug for flac muxers that writs truncated metadata
picture block size if
+// the picture size do not fit in 24 bits. lavf flacenc used to have
the issue and based
+// on broken files some other unknown flac muxer seems to do it also.
+if (truncate_workaround &&
+s->strict_std_compliance <= FF_COMPLIANCE_NORMAL &&
+len > left && (len & 0xff) == left) {
+av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture size
from %u to %u\n", left, len);
+trunclen = len - left;
+} else {
+av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
short\n");
+if (s->error_recognition & AV_EF_EXPLODE)
+ret = AVERROR_INVALIDDATA;
+goto fail;
+}
}
if (!(data = av_buffer_alloc(len + AV_INPUT_BUFFER_PADDING_SIZE))) {
RETURN_ERROR(AVERROR(ENOMEM));
}
-bytestream2_get_bufferu(, data->data, len);
+
+if (trunclen == 0) {
+bytestream2_get_bufferu(, data->data, len);
+} else {
+// If truncation was detected copy all data from block and read
missing bytes
+// not included in the block size
+bytestream2_get_bufferu(, data->data, left);
+if (avio_read(s->pb, data->data + len - trunclen, trunclen) < trunclen)
+RETURN_ERROR(AVERROR_INVALIDDATA);
+}
memset(data->data + len, 0, AV_INPUT_BUFFER_PADDING_SIZE);
if (AV_RB64(data->data) == PNGSIG)
diff --git a/libavformat/flac_picture.h b/libavformat/flac_picture.h
index 4374b6f4f6..61fd0c8806 100644
--- a/libavformat/flac_picture.h
+++ b/libavformat/flac_picture.h
@@ -26,6 +26,6 @@
#define RETURN_ERROR(code) do { ret = (code); goto fail; } while (0)
-int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size);
+int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size, int
truncate_workaround);
#endif /* AVFORMAT_FLAC_PICTURE_H */
diff --git a/libavformat/flacdec.c b/libavformat/flacdec.c
index cb516fb1f3..79c05f14bf 100644
--- a/libavformat/flacdec.c
+++
Re: [FFmpeg-devel] [PATCH v3] libavformat/flacdec: Workaround for truncated metadata picture size
Hi, any chance to get this merged?
On Sun, May 3, 2020 at 12:32 PM Mattias Wadman wrote:
>
> Sorry for nagging, something left to fix?
>
> On Mon, Apr 27, 2020 at 10:10 AM Mattias Wadman
> wrote:
> >
> > Hi again, looks ok?
> >
> > On Fri, Apr 24, 2020 at 12:54 PM Mattias Wadman
> > wrote:
> > >
> > > lavf flacenc could previously write truncated metadata picture size if
> > > the picture data did not fit in 24 bits. Detect this by truncting the
> > > size found inside the picture block and if it matches the block size
> > > use it and read rest of picture data.
> > >
> > > Also only enable this workaround flac files and not ogg files with flac
> > > METADATA_BLOCK_PICTURE comment.
> > >
> > > flacenc was fixed in e447a4d112bcfee10126c54eb4481fa8712957c8
> > > before the fix a broken flac for reproduction could be generated with:
> > > ffmpeg -f lavfi -i sine -f lavfi -i color=red:size=2400x2400 -map 0:0
> > > -map 1:0 -c:v:0 bmp -disposition:1 attached_pic -t 1 test.flac
> > >
> > > Fixes ticket 6333
> > > ---
> > > libavformat/flac_picture.c | 35 +++
> > > libavformat/flac_picture.h | 2 +-
> > > libavformat/flacdec.c| 2 +-
> > > libavformat/oggparsevorbis.c | 2 +-
> > > 4 files changed, 30 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
> > > index 81ddf80465..61277e9dee 100644
> > > --- a/libavformat/flac_picture.c
> > > +++ b/libavformat/flac_picture.c
> > > @@ -27,7 +27,7 @@
> > > #include "id3v2.h"
> > > #include "internal.h"
> > >
> > > -int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
> > > +int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int
> > > buf_size, int truncate_workaround)
> > > {
> > > const CodecMime *mime = ff_id3v2_mime_tags;
> > > enum AVCodecID id = AV_CODEC_ID_NONE;
> > > @@ -36,7 +36,8 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t
> > > *buf, int buf_size)
> > > GetByteContext g;
> > > AVStream *st;
> > > int width, height, ret = 0;
> > > -unsigned int len, type;
> > > +unsigned int type;
> > > +uint32_t len, left, trunclen = 0;
> > >
> > > if (buf_size < 34) {
> > > av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> > > short\n");
> > > @@ -114,16 +115,34 @@ int ff_flac_parse_picture(AVFormatContext *s,
> > > uint8_t *buf, int buf_size)
> > >
> > > /* picture data */
> > > len = bytestream2_get_be32u();
> > > -if (len <= 0 || len > bytestream2_get_bytes_left()) {
> > > -av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> > > short\n");
> > > -if (s->error_recognition & AV_EF_EXPLODE)
> > > -ret = AVERROR_INVALIDDATA;
> > > -goto fail;
> > > +
> > > +left = bytestream2_get_bytes_left();
> > > +if (len <= 0 || len > left) {
> > > +// Workaround lavf flacenc bug that allowed writing truncated
> > > metadata picture block size if
> > > +// picture size did not fit in 24 bits
> > > +if (truncate_workaround && len > left && (len & 0xff) ==
> > > left) {
> > > +av_log(s, AV_LOG_INFO, "Correcting truncated metadata
> > > picture size from %d to %d\n", left, len);
> > > +trunclen = len - left;
> > > +} else {
> > > +av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> > > short\n");
> > > +if (s->error_recognition & AV_EF_EXPLODE)
> > > +ret = AVERROR_INVALIDDATA;
> > > +goto fail;
> > > +}
> > > }
> > > if (!(data = av_buffer_alloc(len + AV_INPUT_BUFFER_PADDING_SIZE))) {
> > > RETURN_ERROR(AVERROR(ENOMEM));
> > > }
> > > -bytestream2_get_bufferu(, data->data, len);
> > > +
> > > +if (trunclen == 0) {
> > > +bytestream2_get_bufferu(, data->data, len);
> > > +} else {
> > > +// If truncation was detect copy all data from block and read
> > > missing bytes
> &g
Re: [FFmpeg-devel] [PATCH v3] libavformat/flacdec: Workaround for truncated metadata picture size
Sorry for nagging, something left to fix?
On Mon, Apr 27, 2020 at 10:10 AM Mattias Wadman
wrote:
>
> Hi again, looks ok?
>
> On Fri, Apr 24, 2020 at 12:54 PM Mattias Wadman
> wrote:
> >
> > lavf flacenc could previously write truncated metadata picture size if
> > the picture data did not fit in 24 bits. Detect this by truncting the
> > size found inside the picture block and if it matches the block size
> > use it and read rest of picture data.
> >
> > Also only enable this workaround flac files and not ogg files with flac
> > METADATA_BLOCK_PICTURE comment.
> >
> > flacenc was fixed in e447a4d112bcfee10126c54eb4481fa8712957c8
> > before the fix a broken flac for reproduction could be generated with:
> > ffmpeg -f lavfi -i sine -f lavfi -i color=red:size=2400x2400 -map 0:0 -map
> > 1:0 -c:v:0 bmp -disposition:1 attached_pic -t 1 test.flac
> >
> > Fixes ticket 6333
> > ---
> > libavformat/flac_picture.c | 35 +++
> > libavformat/flac_picture.h | 2 +-
> > libavformat/flacdec.c| 2 +-
> > libavformat/oggparsevorbis.c | 2 +-
> > 4 files changed, 30 insertions(+), 11 deletions(-)
> >
> > diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
> > index 81ddf80465..61277e9dee 100644
> > --- a/libavformat/flac_picture.c
> > +++ b/libavformat/flac_picture.c
> > @@ -27,7 +27,7 @@
> > #include "id3v2.h"
> > #include "internal.h"
> >
> > -int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
> > +int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size,
> > int truncate_workaround)
> > {
> > const CodecMime *mime = ff_id3v2_mime_tags;
> > enum AVCodecID id = AV_CODEC_ID_NONE;
> > @@ -36,7 +36,8 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t
> > *buf, int buf_size)
> > GetByteContext g;
> > AVStream *st;
> > int width, height, ret = 0;
> > -unsigned int len, type;
> > +unsigned int type;
> > +uint32_t len, left, trunclen = 0;
> >
> > if (buf_size < 34) {
> > av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> > short\n");
> > @@ -114,16 +115,34 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t
> > *buf, int buf_size)
> >
> > /* picture data */
> > len = bytestream2_get_be32u();
> > -if (len <= 0 || len > bytestream2_get_bytes_left()) {
> > -av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> > short\n");
> > -if (s->error_recognition & AV_EF_EXPLODE)
> > -ret = AVERROR_INVALIDDATA;
> > -goto fail;
> > +
> > +left = bytestream2_get_bytes_left();
> > +if (len <= 0 || len > left) {
> > +// Workaround lavf flacenc bug that allowed writing truncated
> > metadata picture block size if
> > +// picture size did not fit in 24 bits
> > +if (truncate_workaround && len > left && (len & 0xff) == left)
> > {
> > +av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture
> > size from %d to %d\n", left, len);
> > +trunclen = len - left;
> > +} else {
> > +av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> > short\n");
> > +if (s->error_recognition & AV_EF_EXPLODE)
> > +ret = AVERROR_INVALIDDATA;
> > +goto fail;
> > +}
> > }
> > if (!(data = av_buffer_alloc(len + AV_INPUT_BUFFER_PADDING_SIZE))) {
> > RETURN_ERROR(AVERROR(ENOMEM));
> > }
> > -bytestream2_get_bufferu(, data->data, len);
> > +
> > +if (trunclen == 0) {
> > +bytestream2_get_bufferu(, data->data, len);
> > +} else {
> > +// If truncation was detect copy all data from block and read
> > missing bytes
> > +// not included in the block size
> > +bytestream2_get_bufferu(, data->data, left);
> > +if (avio_read(s->pb, data->data + len - trunclen, trunclen) <
> > trunclen)
> > +RETURN_ERROR(AVERROR_INVALIDDATA);
> > +}
> > memset(data->data + len, 0, AV_INPUT_BUFFER_PADDING_SIZE);
> >
> > if (AV_RB64(data->data) == PNGSIG)
> > diff --git a/libavformat/flac_picture.h b/libavformat/flac_picture.h
> > index 4374b6f4f6..61fd0c8806
Re: [FFmpeg-devel] [PATCH 2/3] oggdec: verify page checksum
On Tue, Apr 28, 2020 at 4:45 PM Lynne wrote: > > Apr 28, 2020, 15:31 by mattias.wad...@gmail.com: > > > Nice, test files works fine now > > > > Would it make sense to conditionally ignore crc mismatch based on > > s->error_recognition & AV_EF_CRCCHECK ? > > > > I don't think so. This container specifically relies on CRC matching to > identify packets > during a seek. While other containers have more advanced sync mechanisms > beyond a simple > syncword and checksum, that's all we have here. > What's worse, we need to be able to handle concatenated ogg files (chained > opus, vorbis, etc.), > which are widely used on internet radios. Those have the extradata needed to > configure the decoder > on the first packet. If we skip the CRC and misidentify a packet as a header, > we'll misconfigure the > decoder and break decoding until the next actual header arrives, which could > be many minutes. > The whole chained ogg mechanism is already fragile enough as it unfortunately > is. Sorry yes that make sense. I meant more that AV_EF_CRCCHECK seems to be set by default so adding a conditionally check would be if someone for some reason really want to skip it using -err_detect 0 or so. > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/3] oggdec: verify page checksum
On Tue, Apr 28, 2020 at 4:12 PM Lynne wrote: > > Apr 28, 2020, 14:05 by mattias.wad...@gmail.com: > > > On Tue, Apr 28, 2020 at 1:59 PM Lynne wrote: > > > >> > >> This makes decoding far more robust, since OggS, the ogg magic, > >> can be commonly found randomly in streams, which previously made > >> the demuxer think there's a new stream or a change in such. > >> > >> Patch attached. > >> > > > > Maybe nitpick, could seek back even longer than size on crc mismatch? > > > > Thanks a lot for reviewing the patches. > I was thinking about seeking back further to perhaps the version byte, but > unfortunately, that's > not possible. If ffio_ensure_seekback is called multiple times, the last call > overwrites the previous > calls and we lose the ability to seek before it. > Since the only place at which we know the exact size of the page is when its > signaled, that's the > only point we can ensure we can seek back to. > Before that, we can seek back from the header's end to the version byte. > Unfortunately, that > would mean verifying the header before the checksum, which as you've pointed > out, is bad > for robustness. > Still, this is definitely an improvement, since previously the demuxer didn't > seek back at all. I see, thanks for the explaining. Also better than in my patch where it would skip the whole page. > Anyway, I've implemented checking the version byte after reading the CRC as > you suggested > and have attached the new patch. Nice, test files works fine now Would it make sense to conditionally ignore crc mismatch based on s->error_recognition & AV_EF_CRCCHECK ? > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/3] oggdec: verify page checksum
On Tue, Apr 28, 2020 at 1:59 PM Lynne wrote: > > This makes decoding far more robust, since OggS, the ogg magic, > can be commonly found randomly in streams, which previously made > the demuxer think there's a new stream or a change in such. > > Patch attached. Maybe nitpick, could seek back even longer than size on crc mismatch? > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/3] oggdec: verify page checksum
On Tue, Apr 28, 2020 at 1:59 PM Lynne wrote: > > This makes decoding far more robust, since OggS, the ogg magic, > can be commonly found randomly in streams, which previously made > the demuxer think there's a new stream or a change in such. > > Patch attached. Should check version after verifying checksum? this breaks with my test files were the false syncword has a non-zero byte afterwards. > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avformat/oggdec: Add page CRC verification
On Tue, Apr 28, 2020 at 2:01 PM Lynne wrote: > > Apr 27, 2020, 21:55 by mich...@niedermayer.cc: > > > On Mon, Apr 27, 2020 at 05:19:49PM +0200, Mattias Wadman wrote: > > > >> Fixes seek issue with ogg files that have segment data that happens to be > >> encoded as a "OggS" page syncword. Very unlikely but seems to happen. > >> > >> Have been observed to happen with ffmpeg+libvorbis and oggenc encoding > >> to 441khz stereo at 160kbps. > >> --- > >> libavformat/oggdec.c | 96 +--- > >> 1 file changed, 73 insertions(+), 23 deletions(-) > >> > > > > breaks chained ogg > > > > ./ffmpeg -threads 1 -i ~/tickets/868/chained_streams.ogg -f null - > > > > frame= 954 fps=0.0 q=-0.0 Lsize=N/A time=00:00:31.79 bitrate=N/A speed= > > 105x > > vs > > frame= 120 fps=0.0 q=-0.0 Lsize=N/A time=00:00:03.99 bitrate=N/A speed= > > 289x > > > > stream should be here: > > http://v2v.cc/~j/theora_testsuite/chained_streams.ogg > > > > thx > > > > I've sent a patchset which amongst other things, implements this cleanly and > does not break that file. > Had this for a long time, wanted to finish Opus chaining, but seeing this > made me send it to the list. Aha thanks. I should have asked on the list before starting, but i learned a lot. I will review your patches and try with some files that have the issue. Unfortunately i can't share them because of legal reasons. > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avformat/oggdec: Add page CRC verification
On Tue, Apr 28, 2020 at 1:06 PM Mattias Wadman wrote:
>
> Fixes seek issue with ogg files that have segment data that happens to be
> encoded as a "OggS" page syncword. Very unlikely but seems to happen.
>
> Have been observed to happen with ffmpeg+libvorbis and oggenc encoding
> to 441khz stereo at 160kbps.
> ---
> libavformat/oggdec.c | 136 +--
> 1 file changed, 92 insertions(+), 44 deletions(-)
>
> diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
> index 95190589ab..0213f1fa12 100644
> --- a/libavformat/oggdec.c
> +++ b/libavformat/oggdec.c
> @@ -31,6 +31,8 @@
> #include
> #include "libavutil/avassert.h"
> #include "libavutil/intreadwrite.h"
> +#include "libavutil/crc.h"
> +#include "libavcodec/bytestream.h"
> #include "oggdec.h"
> #include "avformat.h"
> #include "internal.h"
> @@ -205,32 +207,30 @@ static const struct ogg_codec *ogg_find_codec(uint8_t
> *buf, int size)
> * situation where a new audio stream spawn (identified with a new serial)
> and
> * must replace the previous one (track switch).
> */
> -static int ogg_replace_stream(AVFormatContext *s, uint32_t serial, int nsegs)
> +static int ogg_replace_stream(AVFormatContext *s, uint32_t serial, uint8_t
> *segmentsdata, int size)
> {
> struct ogg *ogg = s->priv_data;
> struct ogg_stream *os;
> const struct ogg_codec *codec;
> int i = 0;
> +int magiclen = 8;
>
> -if (s->pb->seekable & AVIO_SEEKABLE_NORMAL) {
> -uint8_t magic[8];
> -int64_t pos = avio_tell(s->pb);
> -avio_skip(s->pb, nsegs);
> -if (avio_read(s->pb, magic, sizeof(magic)) != sizeof(magic))
> -return AVERROR_INVALIDDATA;
> -avio_seek(s->pb, pos, SEEK_SET);
> -codec = ogg_find_codec(magic, sizeof(magic));
> -if (!codec) {
> -av_log(s, AV_LOG_ERROR, "Cannot identify new stream\n");
> -return AVERROR_INVALIDDATA;
> -}
> -for (i = 0; i < ogg->nstreams; i++) {
> -if (ogg->streams[i].codec == codec)
> -break;
> -}
> -if (i >= ogg->nstreams)
> -return ogg_new_stream(s, serial);
> -} else if (ogg->nstreams != 1) {
> +if (size < magiclen)
> +return AVERROR_INVALIDDATA;
> +
> +codec = ogg_find_codec(segmentsdata, magiclen);
> +if (!codec) {
> +av_log(s, AV_LOG_ERROR, "Cannot identify new stream\n");
> +return AVERROR_INVALIDDATA;
> +}
> +for (i = 0; i < ogg->nstreams; i++) {
> +if (ogg->streams[i].codec == codec)
> +break;
> +}
> +if (i >= ogg->nstreams)
> +return ogg_new_stream(s, serial);
> +
> +if (ogg->nstreams != 1) {
> avpriv_report_missing_feature(s, "Changing stream parameters in
> multistream ogg");
> return AVERROR_PATCHWELCOME;
> }
Not sure what the correct behaviour should be here now. Currently this
check is only done if
the io context is not seekable. But now ogg_replace_stream does need
to seek as it has all data.
> @@ -339,14 +339,24 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
> AVIOContext *bc = s->pb;
> struct ogg *ogg = s->priv_data;
> struct ogg_stream *os;
> -int ret, i = 0;
> -int flags, nsegs;
> +int ret, i;
> +int version, flags, nsegs;
> uint64_t gp;
> uint32_t serial;
> +uint32_t crc;
> int size, idx;
> uint8_t sync[4];
> -int sp = 0;
> -
> +uint8_t header[23];
> +GetByteContext headergb;
> +uint8_t segments[255];
> +uint8_t *segmentsdata;
> +int sp;
> +const AVCRC *crc_table;
> +uint32_t ccrc;
> +
> +again:
> +sp = 0;
> +i = 0;
> ret = avio_read(bc, sync, 4);
> if (ret < 4)
> return ret < 0 ? ret : AVERROR_EOF;
> @@ -378,47 +388,87 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
> return AVERROR_INVALIDDATA;
> }
>
> -if (avio_r8(bc) != 0) { /* version */
> -av_log (s, AV_LOG_ERROR, "ogg page, unsupported version\n");
> +ret = avio_read(bc, header, sizeof(header));
> +if (ret < sizeof(header))
> +return AVERROR_INVALIDDATA;
> +nsegs = header[22];
> +ret = avio_read(bc, segments, nsegs);
> +if (ret < nsegs)
> +return AVERROR_INVALIDDATA;
> +size = 0;
> +for (i = 0; i < nsegs; i++)
> +size += segments[i];
&
[FFmpeg-devel] [PATCH v2] avformat/oggdec: Add page CRC verification
Fixes seek issue with ogg files that have segment data that happens to be
encoded as a "OggS" page syncword. Very unlikely but seems to happen.
Have been observed to happen with ffmpeg+libvorbis and oggenc encoding
to 441khz stereo at 160kbps.
---
libavformat/oggdec.c | 136 +--
1 file changed, 92 insertions(+), 44 deletions(-)
diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index 95190589ab..0213f1fa12 100644
--- a/libavformat/oggdec.c
+++ b/libavformat/oggdec.c
@@ -31,6 +31,8 @@
#include
#include "libavutil/avassert.h"
#include "libavutil/intreadwrite.h"
+#include "libavutil/crc.h"
+#include "libavcodec/bytestream.h"
#include "oggdec.h"
#include "avformat.h"
#include "internal.h"
@@ -205,32 +207,30 @@ static const struct ogg_codec *ogg_find_codec(uint8_t
*buf, int size)
* situation where a new audio stream spawn (identified with a new serial) and
* must replace the previous one (track switch).
*/
-static int ogg_replace_stream(AVFormatContext *s, uint32_t serial, int nsegs)
+static int ogg_replace_stream(AVFormatContext *s, uint32_t serial, uint8_t
*segmentsdata, int size)
{
struct ogg *ogg = s->priv_data;
struct ogg_stream *os;
const struct ogg_codec *codec;
int i = 0;
+int magiclen = 8;
-if (s->pb->seekable & AVIO_SEEKABLE_NORMAL) {
-uint8_t magic[8];
-int64_t pos = avio_tell(s->pb);
-avio_skip(s->pb, nsegs);
-if (avio_read(s->pb, magic, sizeof(magic)) != sizeof(magic))
-return AVERROR_INVALIDDATA;
-avio_seek(s->pb, pos, SEEK_SET);
-codec = ogg_find_codec(magic, sizeof(magic));
-if (!codec) {
-av_log(s, AV_LOG_ERROR, "Cannot identify new stream\n");
-return AVERROR_INVALIDDATA;
-}
-for (i = 0; i < ogg->nstreams; i++) {
-if (ogg->streams[i].codec == codec)
-break;
-}
-if (i >= ogg->nstreams)
-return ogg_new_stream(s, serial);
-} else if (ogg->nstreams != 1) {
+if (size < magiclen)
+return AVERROR_INVALIDDATA;
+
+codec = ogg_find_codec(segmentsdata, magiclen);
+if (!codec) {
+av_log(s, AV_LOG_ERROR, "Cannot identify new stream\n");
+return AVERROR_INVALIDDATA;
+}
+for (i = 0; i < ogg->nstreams; i++) {
+if (ogg->streams[i].codec == codec)
+break;
+}
+if (i >= ogg->nstreams)
+return ogg_new_stream(s, serial);
+
+if (ogg->nstreams != 1) {
avpriv_report_missing_feature(s, "Changing stream parameters in
multistream ogg");
return AVERROR_PATCHWELCOME;
}
@@ -339,14 +339,24 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
AVIOContext *bc = s->pb;
struct ogg *ogg = s->priv_data;
struct ogg_stream *os;
-int ret, i = 0;
-int flags, nsegs;
+int ret, i;
+int version, flags, nsegs;
uint64_t gp;
uint32_t serial;
+uint32_t crc;
int size, idx;
uint8_t sync[4];
-int sp = 0;
-
+uint8_t header[23];
+GetByteContext headergb;
+uint8_t segments[255];
+uint8_t *segmentsdata;
+int sp;
+const AVCRC *crc_table;
+uint32_t ccrc;
+
+again:
+sp = 0;
+i = 0;
ret = avio_read(bc, sync, 4);
if (ret < 4)
return ret < 0 ? ret : AVERROR_EOF;
@@ -378,47 +388,87 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
return AVERROR_INVALIDDATA;
}
-if (avio_r8(bc) != 0) { /* version */
-av_log (s, AV_LOG_ERROR, "ogg page, unsupported version\n");
+ret = avio_read(bc, header, sizeof(header));
+if (ret < sizeof(header))
+return AVERROR_INVALIDDATA;
+nsegs = header[22];
+ret = avio_read(bc, segments, nsegs);
+if (ret < nsegs)
+return AVERROR_INVALIDDATA;
+size = 0;
+for (i = 0; i < nsegs; i++)
+size += segments[i];
+
+bytestream2_init(, header, sizeof(header));
+version = bytestream2_get_byte();
+flags = bytestream2_get_byte();
+gp = bytestream2_get_le64();
+serial = bytestream2_get_le32();
+bytestream2_skip(, 4); // seq le32
+crc = bytestream2_get_le32();
+
+segmentsdata = av_malloc(size);
+if (!segmentsdata)
+return AVERROR(ENOMEM);
+ret = avio_read(bc, segmentsdata, size);
+if (ret < size) {
+av_freep();
return AVERROR_INVALIDDATA;
}
-flags = avio_r8(bc);
-gp = avio_rl64(bc);
-serial = avio_rl32(bc);
-avio_skip(bc, 8); /* seq, crc */
-nsegs = avio_r8(bc);
+// Reset CRC in header to zero and calculate for whole page
+crc_table = av_crc_get_table(AV_CRC_32_IEEE);
+memset([18], 0, 4);
+ccrc = 0;
+ccrc = av_crc(crc_table, ccrc, "OggS", 4);
+ccrc = av_crc(crc_table, ccrc, header, sizeof(header));
+ccrc = av_crc(crc_table, ccrc, segments, nsegs);
+ccrc = av_crc(crc_table, ccrc, segmentsdata, size);
+// Default AV_CRC_32_IEEE
[FFmpeg-devel] [PATCH] avformat/oggdec: Add page CRC verification
Fixes seek issue with ogg files that have segment data that happens to be
encoded as a "OggS" page syncword. Very unlikely but seems to happen.
Have been observed to happen with ffmpeg+libvorbis and oggenc encoding
to 441khz stereo at 160kbps.
---
libavformat/oggdec.c | 96 +---
1 file changed, 73 insertions(+), 23 deletions(-)
diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index 95190589ab..22532f0341 100644
--- a/libavformat/oggdec.c
+++ b/libavformat/oggdec.c
@@ -31,6 +31,8 @@
#include
#include "libavutil/avassert.h"
#include "libavutil/intreadwrite.h"
+#include "libavutil/crc.h"
+#include "libavcodec/bytestream.h"
#include "oggdec.h"
#include "avformat.h"
#include "internal.h"
@@ -339,14 +341,24 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
AVIOContext *bc = s->pb;
struct ogg *ogg = s->priv_data;
struct ogg_stream *os;
-int ret, i = 0;
-int flags, nsegs;
+int ret, i;
+int version, flags, nsegs;
uint64_t gp;
uint32_t serial;
+uint32_t crc;
int size, idx;
uint8_t sync[4];
-int sp = 0;
-
+uint8_t header[23];
+GetByteContext headergb;
+uint8_t segments[255];
+uint8_t *segmentsdata;
+int sp;
+const AVCRC *crc_table;
+uint32_t ccrc;
+
+again:
+sp = 0;
+i = 0;
ret = avio_read(bc, sync, 4);
if (ret < 4)
return ret < 0 ? ret : AVERROR_EOF;
@@ -378,19 +390,59 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
return AVERROR_INVALIDDATA;
}
-if (avio_r8(bc) != 0) { /* version */
-av_log (s, AV_LOG_ERROR, "ogg page, unsupported version\n");
+ret = avio_read(bc, header, sizeof(header));
+if (ret < sizeof(header))
+return AVERROR_INVALIDDATA;
+nsegs = header[22];
+ret = avio_read(bc, segments, nsegs);
+if (ret < nsegs)
+return AVERROR_INVALIDDATA;
+size = 0;
+for (i = 0; i < nsegs; i++)
+size += segments[i];
+
+bytestream2_init(, header, sizeof(header));
+version = bytestream2_get_byte();
+flags = bytestream2_get_byte();
+gp = bytestream2_get_le64();
+serial = bytestream2_get_le32();
+bytestream2_skip(, 4); // seq le32
+crc = bytestream2_get_le32();
+
+segmentsdata = av_malloc(size);
+if (!segmentsdata)
+return AVERROR(ENOMEM);
+ret = avio_read(bc, segmentsdata, size);
+if (ret < size) {
+av_freep();
return AVERROR_INVALIDDATA;
}
-flags = avio_r8(bc);
-gp = avio_rl64(bc);
-serial = avio_rl32(bc);
-avio_skip(bc, 8); /* seq, crc */
-nsegs = avio_r8(bc);
+// Reset CRC in header to zero and calculate for whole page
+crc_table = av_crc_get_table(AV_CRC_32_IEEE);
+memset([18], 0, 4);
+ccrc = 0;
+ccrc = av_crc(crc_table, ccrc, "OggS", 4);
+ccrc = av_crc(crc_table, ccrc, header, sizeof(header));
+ccrc = av_crc(crc_table, ccrc, segments, nsegs);
+ccrc = av_crc(crc_table, ccrc, segmentsdata, size);
+// Default AV_CRC_32_IEEE table is BE
+ccrc = av_bswap32(ccrc);
+
+if (ccrc != crc) {
+av_log(s, AV_LOG_TRACE, "ogg page, invalid checksum %x != %x\n", ccrc,
crc);
+if (s->error_recognition & AV_EF_CRCCHECK) {
+av_freep();
+// TODO: smarter use of read data?
+goto again;
+}
+}
-if (avio_feof(bc))
-return AVERROR_EOF;
+if (version != 0) {
+av_log(s, AV_LOG_ERROR, "ogg page, unsupported version %d\n", version);
+av_freep();
+return AVERROR_INVALIDDATA;
+}
idx = ogg_find_stream(ogg, serial);
if (idx < 0) {
@@ -401,24 +453,24 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
if (idx < 0) {
av_log(s, AV_LOG_ERROR, "failed to create or replace stream\n");
+av_freep();
return idx;
}
}
os = ogg->streams + idx;
ogg->page_pos =
-os->page_pos = avio_tell(bc) - 27;
+os->page_pos = avio_tell(bc) - 27 - size - nsegs;;
if (os->psize > 0) {
ret = ogg_new_buf(ogg, idx);
-if (ret < 0)
+if (ret < 0) {
+av_freep();
return ret;
+}
}
-ret = avio_read(bc, os->segments, nsegs);
-if (ret < nsegs)
-return ret < 0 ? ret : AVERROR_EOF;
-
+memcpy(os->segments, segments, nsegs);
os->nsegs = nsegs;
os->segp = 0;
@@ -456,10 +508,8 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
os->buf = nb;
}
-ret = avio_read(bc, os->buf + os->bufpos, size);
-if (ret < size)
-return ret < 0 ? ret : AVERROR_EOF;
-
+memcpy(os->buf + os->bufpos, segmentsdata, size);
+av_freep();
os->bufpos += size;
os->granule = gp;
os->flags = flags;
--
2.18.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
Re: [FFmpeg-devel] [PATCH v3] libavformat/flacdec: Workaround for truncated metadata picture size
Hi again, looks ok?
On Fri, Apr 24, 2020 at 12:54 PM Mattias Wadman
wrote:
>
> lavf flacenc could previously write truncated metadata picture size if
> the picture data did not fit in 24 bits. Detect this by truncting the
> size found inside the picture block and if it matches the block size
> use it and read rest of picture data.
>
> Also only enable this workaround flac files and not ogg files with flac
> METADATA_BLOCK_PICTURE comment.
>
> flacenc was fixed in e447a4d112bcfee10126c54eb4481fa8712957c8
> before the fix a broken flac for reproduction could be generated with:
> ffmpeg -f lavfi -i sine -f lavfi -i color=red:size=2400x2400 -map 0:0 -map
> 1:0 -c:v:0 bmp -disposition:1 attached_pic -t 1 test.flac
>
> Fixes ticket 6333
> ---
> libavformat/flac_picture.c | 35 +++
> libavformat/flac_picture.h | 2 +-
> libavformat/flacdec.c| 2 +-
> libavformat/oggparsevorbis.c | 2 +-
> 4 files changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
> index 81ddf80465..61277e9dee 100644
> --- a/libavformat/flac_picture.c
> +++ b/libavformat/flac_picture.c
> @@ -27,7 +27,7 @@
> #include "id3v2.h"
> #include "internal.h"
>
> -int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
> +int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size,
> int truncate_workaround)
> {
> const CodecMime *mime = ff_id3v2_mime_tags;
> enum AVCodecID id = AV_CODEC_ID_NONE;
> @@ -36,7 +36,8 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf,
> int buf_size)
> GetByteContext g;
> AVStream *st;
> int width, height, ret = 0;
> -unsigned int len, type;
> +unsigned int type;
> +uint32_t len, left, trunclen = 0;
>
> if (buf_size < 34) {
> av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> short\n");
> @@ -114,16 +115,34 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t
> *buf, int buf_size)
>
> /* picture data */
> len = bytestream2_get_be32u();
> -if (len <= 0 || len > bytestream2_get_bytes_left()) {
> -av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> short\n");
> -if (s->error_recognition & AV_EF_EXPLODE)
> -ret = AVERROR_INVALIDDATA;
> -goto fail;
> +
> +left = bytestream2_get_bytes_left();
> +if (len <= 0 || len > left) {
> +// Workaround lavf flacenc bug that allowed writing truncated
> metadata picture block size if
> +// picture size did not fit in 24 bits
> +if (truncate_workaround && len > left && (len & 0xff) == left) {
> +av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture
> size from %d to %d\n", left, len);
> +trunclen = len - left;
> +} else {
> +av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> short\n");
> +if (s->error_recognition & AV_EF_EXPLODE)
> +ret = AVERROR_INVALIDDATA;
> +goto fail;
> +}
> }
> if (!(data = av_buffer_alloc(len + AV_INPUT_BUFFER_PADDING_SIZE))) {
> RETURN_ERROR(AVERROR(ENOMEM));
> }
> -bytestream2_get_bufferu(, data->data, len);
> +
> +if (trunclen == 0) {
> +bytestream2_get_bufferu(, data->data, len);
> +} else {
> +// If truncation was detect copy all data from block and read
> missing bytes
> +// not included in the block size
> +bytestream2_get_bufferu(, data->data, left);
> +if (avio_read(s->pb, data->data + len - trunclen, trunclen) <
> trunclen)
> +RETURN_ERROR(AVERROR_INVALIDDATA);
> +}
> memset(data->data + len, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>
> if (AV_RB64(data->data) == PNGSIG)
> diff --git a/libavformat/flac_picture.h b/libavformat/flac_picture.h
> index 4374b6f4f6..61fd0c8806 100644
> --- a/libavformat/flac_picture.h
> +++ b/libavformat/flac_picture.h
> @@ -26,6 +26,6 @@
>
> #define RETURN_ERROR(code) do { ret = (code); goto fail; } while (0)
>
> -int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size);
> +int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size,
> int truncate_workaround);
>
> #endif /* AVFORMAT_FLAC_PICTURE_H */
> diff --git a/libavformat/flacdec.c b/libavformat/flacdec.c
> index cb516fb1f3..79c05f14bf 100644
> --- a/libavformat/flacdec.c
> +++ b/libavformat/flacdec.c
> @@ -146,7 +146,7 @@ static int flac_read_
Re: [FFmpeg-devel] [PATCH v2] libavformat/flacdec: Workaround for truncated metadata picture size
On Fri, Apr 24, 2020 at 12:41 PM Mattias Wadman
wrote:
>
> lavf flacenc could previously write truncated metadata picture size if
> the picture data did not fit in 24 bits. Detect this by truncting the
> size found inside the picture block and if it matches the block size
> use it and read rest of picture data.
>
> Also only enable this workaround flac files and not ogg files with flac
> METADATA_BLOCK_PICTURE comment.
>
> flacenc was fixed in e447a4d112bcfee10126c54eb4481fa8712957c8
> before the fix a broken flac for reproduction could be generated with:
> ffmpeg -f lavfi -i sine -f lavfi -i color=red:size=2400x2400 -map 0:0 -map
> 1:0 -c:v:0 bmp -disposition:1 attached_pic -t 1 test.flac
>
> Fixes ticket 6333
> ---
> libavformat/flac_picture.c | 31 +++
> libavformat/flac_picture.h | 2 +-
> libavformat/flacdec.c| 2 +-
> libavformat/oggparsevorbis.c | 2 +-
> 4 files changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
> index 81ddf80465..64be9f833d 100644
> --- a/libavformat/flac_picture.c
> +++ b/libavformat/flac_picture.c
> @@ -27,7 +27,7 @@
> #include "id3v2.h"
> #include "internal.h"
>
> -int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
> +int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size,
> int truncate_workaround)
> {
> const CodecMime *mime = ff_id3v2_mime_tags;
> enum AVCodecID id = AV_CODEC_ID_NONE;
> @@ -36,7 +36,8 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf,
> int buf_size)
> GetByteContext g;
> AVStream *st;
> int width, height, ret = 0;
> -unsigned int len, type;
> +unsigned int type;
> +uint32_t len, left, trunclen = 0;
>
> if (buf_size < 34) {
> av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> short\n");
> @@ -114,16 +115,30 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t
> *buf, int buf_size)
>
> /* picture data */
> len = bytestream2_get_be32u();
> -if (len <= 0 || len > bytestream2_get_bytes_left()) {
> -av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> short\n");
> -if (s->error_recognition & AV_EF_EXPLODE)
> -ret = AVERROR_INVALIDDATA;
> -goto fail;
> +
> +left = bytestream2_get_bytes_left();
> +if (len <= 0 || len > left) {
> +// Workaround lavf flacenc bug that allowed writing truncated
> metadata picture block size if
> +// picture size did not fit in 24 bits
> +if (truncate_workaround && len > left && (len & 0xff) == left) {
> +av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture
> size from %d to %d\n", left, len);
> +trunclen = len - left;
> +} else {
> +av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> short\n");
> +if (s->error_recognition & AV_EF_EXPLODE)
> +ret = AVERROR_INVALIDDATA;
> +goto fail;
> +}
> }
> if (!(data = av_buffer_alloc(len + AV_INPUT_BUFFER_PADDING_SIZE))) {
> RETURN_ERROR(AVERROR(ENOMEM));
> }
> -bytestream2_get_bufferu(, data->data, len);
> +bytestream2_get_bufferu(, data->data, left);
Maybe this should not be changed in the non-truncated case? now if for
some reason the picture
size if smaller than block size will include garbage/padding bytes at
the end of the block i think
> +// If truncation was detect read bytes missing in the block size
> +if (trunclen > 0) {
> +if (avio_read(s->pb, data->data + len - trunclen, trunclen) <
> trunclen)
> +RETURN_ERROR(AVERROR_INVALIDDATA);
> +}
> memset(data->data + len, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>
> if (AV_RB64(data->data) == PNGSIG)
> diff --git a/libavformat/flac_picture.h b/libavformat/flac_picture.h
> index 4374b6f4f6..61fd0c8806 100644
> --- a/libavformat/flac_picture.h
> +++ b/libavformat/flac_picture.h
> @@ -26,6 +26,6 @@
>
> #define RETURN_ERROR(code) do { ret = (code); goto fail; } while (0)
>
> -int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size);
> +int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size,
> int truncate_workaround);
>
> #endif /* AVFORMAT_FLAC_PICTURE_H */
> diff --git a/libavformat/flacdec.c b/libavformat/flacdec.c
> index cb516fb1f3..79c05f14bf 100644
> --- a/libavformat/flacdec.c
> +++ b/libavformat/flacdec.c
> @@ -146,7 +146,7 @@ stati
[FFmpeg-devel] [PATCH v3] libavformat/flacdec: Workaround for truncated metadata picture size
lavf flacenc could previously write truncated metadata picture size if
the picture data did not fit in 24 bits. Detect this by truncting the
size found inside the picture block and if it matches the block size
use it and read rest of picture data.
Also only enable this workaround flac files and not ogg files with flac
METADATA_BLOCK_PICTURE comment.
flacenc was fixed in e447a4d112bcfee10126c54eb4481fa8712957c8
before the fix a broken flac for reproduction could be generated with:
ffmpeg -f lavfi -i sine -f lavfi -i color=red:size=2400x2400 -map 0:0 -map 1:0
-c:v:0 bmp -disposition:1 attached_pic -t 1 test.flac
Fixes ticket 6333
---
libavformat/flac_picture.c | 35 +++
libavformat/flac_picture.h | 2 +-
libavformat/flacdec.c| 2 +-
libavformat/oggparsevorbis.c | 2 +-
4 files changed, 30 insertions(+), 11 deletions(-)
diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
index 81ddf80465..61277e9dee 100644
--- a/libavformat/flac_picture.c
+++ b/libavformat/flac_picture.c
@@ -27,7 +27,7 @@
#include "id3v2.h"
#include "internal.h"
-int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
+int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size, int
truncate_workaround)
{
const CodecMime *mime = ff_id3v2_mime_tags;
enum AVCodecID id = AV_CODEC_ID_NONE;
@@ -36,7 +36,8 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf,
int buf_size)
GetByteContext g;
AVStream *st;
int width, height, ret = 0;
-unsigned int len, type;
+unsigned int type;
+uint32_t len, left, trunclen = 0;
if (buf_size < 34) {
av_log(s, AV_LOG_ERROR, "Attached picture metadata block too short\n");
@@ -114,16 +115,34 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t
*buf, int buf_size)
/* picture data */
len = bytestream2_get_be32u();
-if (len <= 0 || len > bytestream2_get_bytes_left()) {
-av_log(s, AV_LOG_ERROR, "Attached picture metadata block too short\n");
-if (s->error_recognition & AV_EF_EXPLODE)
-ret = AVERROR_INVALIDDATA;
-goto fail;
+
+left = bytestream2_get_bytes_left();
+if (len <= 0 || len > left) {
+// Workaround lavf flacenc bug that allowed writing truncated metadata
picture block size if
+// picture size did not fit in 24 bits
+if (truncate_workaround && len > left && (len & 0xff) == left) {
+av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture size
from %d to %d\n", left, len);
+trunclen = len - left;
+} else {
+av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
short\n");
+if (s->error_recognition & AV_EF_EXPLODE)
+ret = AVERROR_INVALIDDATA;
+goto fail;
+}
}
if (!(data = av_buffer_alloc(len + AV_INPUT_BUFFER_PADDING_SIZE))) {
RETURN_ERROR(AVERROR(ENOMEM));
}
-bytestream2_get_bufferu(, data->data, len);
+
+if (trunclen == 0) {
+bytestream2_get_bufferu(, data->data, len);
+} else {
+// If truncation was detect copy all data from block and read missing
bytes
+// not included in the block size
+bytestream2_get_bufferu(, data->data, left);
+if (avio_read(s->pb, data->data + len - trunclen, trunclen) < trunclen)
+RETURN_ERROR(AVERROR_INVALIDDATA);
+}
memset(data->data + len, 0, AV_INPUT_BUFFER_PADDING_SIZE);
if (AV_RB64(data->data) == PNGSIG)
diff --git a/libavformat/flac_picture.h b/libavformat/flac_picture.h
index 4374b6f4f6..61fd0c8806 100644
--- a/libavformat/flac_picture.h
+++ b/libavformat/flac_picture.h
@@ -26,6 +26,6 @@
#define RETURN_ERROR(code) do { ret = (code); goto fail; } while (0)
-int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size);
+int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size, int
truncate_workaround);
#endif /* AVFORMAT_FLAC_PICTURE_H */
diff --git a/libavformat/flacdec.c b/libavformat/flacdec.c
index cb516fb1f3..79c05f14bf 100644
--- a/libavformat/flacdec.c
+++ b/libavformat/flacdec.c
@@ -146,7 +146,7 @@ static int flac_read_header(AVFormatContext *s)
}
av_freep();
} else if (metadata_type == FLAC_METADATA_TYPE_PICTURE) {
-ret = ff_flac_parse_picture(s, buffer, metadata_size);
+ret = ff_flac_parse_picture(s, buffer, metadata_size, 1);
av_freep();
if (ret < 0) {
av_log(s, AV_LOG_ERROR, "Error parsing attached picture.\n");
diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
index 27d2c686b6..6f15204ada 100644
--- a/libavformat/oggparsevorbis.c
+++ b/libavformat/oggparsevorbis.c
@@ -165,7 +165,7 @@ int ff_vorbis_comment(AVFormatContext *as, AVDictionary **m,
av_freep();
av_freep();
if (ret > 0)
-
[FFmpeg-devel] [PATCH v2] libavformat/flacdec: Workaround for truncated metadata picture size
lavf flacenc could previously write truncated metadata picture size if
the picture data did not fit in 24 bits. Detect this by truncting the
size found inside the picture block and if it matches the block size
use it and read rest of picture data.
Also only enable this workaround flac files and not ogg files with flac
METADATA_BLOCK_PICTURE comment.
flacenc was fixed in e447a4d112bcfee10126c54eb4481fa8712957c8
before the fix a broken flac for reproduction could be generated with:
ffmpeg -f lavfi -i sine -f lavfi -i color=red:size=2400x2400 -map 0:0 -map 1:0
-c:v:0 bmp -disposition:1 attached_pic -t 1 test.flac
Fixes ticket 6333
---
libavformat/flac_picture.c | 31 +++
libavformat/flac_picture.h | 2 +-
libavformat/flacdec.c| 2 +-
libavformat/oggparsevorbis.c | 2 +-
4 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
index 81ddf80465..64be9f833d 100644
--- a/libavformat/flac_picture.c
+++ b/libavformat/flac_picture.c
@@ -27,7 +27,7 @@
#include "id3v2.h"
#include "internal.h"
-int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
+int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size, int
truncate_workaround)
{
const CodecMime *mime = ff_id3v2_mime_tags;
enum AVCodecID id = AV_CODEC_ID_NONE;
@@ -36,7 +36,8 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf,
int buf_size)
GetByteContext g;
AVStream *st;
int width, height, ret = 0;
-unsigned int len, type;
+unsigned int type;
+uint32_t len, left, trunclen = 0;
if (buf_size < 34) {
av_log(s, AV_LOG_ERROR, "Attached picture metadata block too short\n");
@@ -114,16 +115,30 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t
*buf, int buf_size)
/* picture data */
len = bytestream2_get_be32u();
-if (len <= 0 || len > bytestream2_get_bytes_left()) {
-av_log(s, AV_LOG_ERROR, "Attached picture metadata block too short\n");
-if (s->error_recognition & AV_EF_EXPLODE)
-ret = AVERROR_INVALIDDATA;
-goto fail;
+
+left = bytestream2_get_bytes_left();
+if (len <= 0 || len > left) {
+// Workaround lavf flacenc bug that allowed writing truncated metadata
picture block size if
+// picture size did not fit in 24 bits
+if (truncate_workaround && len > left && (len & 0xff) == left) {
+av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture size
from %d to %d\n", left, len);
+trunclen = len - left;
+} else {
+av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
short\n");
+if (s->error_recognition & AV_EF_EXPLODE)
+ret = AVERROR_INVALIDDATA;
+goto fail;
+}
}
if (!(data = av_buffer_alloc(len + AV_INPUT_BUFFER_PADDING_SIZE))) {
RETURN_ERROR(AVERROR(ENOMEM));
}
-bytestream2_get_bufferu(, data->data, len);
+bytestream2_get_bufferu(, data->data, left);
+// If truncation was detect read bytes missing in the block size
+if (trunclen > 0) {
+if (avio_read(s->pb, data->data + len - trunclen, trunclen) < trunclen)
+RETURN_ERROR(AVERROR_INVALIDDATA);
+}
memset(data->data + len, 0, AV_INPUT_BUFFER_PADDING_SIZE);
if (AV_RB64(data->data) == PNGSIG)
diff --git a/libavformat/flac_picture.h b/libavformat/flac_picture.h
index 4374b6f4f6..61fd0c8806 100644
--- a/libavformat/flac_picture.h
+++ b/libavformat/flac_picture.h
@@ -26,6 +26,6 @@
#define RETURN_ERROR(code) do { ret = (code); goto fail; } while (0)
-int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size);
+int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size, int
truncate_workaround);
#endif /* AVFORMAT_FLAC_PICTURE_H */
diff --git a/libavformat/flacdec.c b/libavformat/flacdec.c
index cb516fb1f3..79c05f14bf 100644
--- a/libavformat/flacdec.c
+++ b/libavformat/flacdec.c
@@ -146,7 +146,7 @@ static int flac_read_header(AVFormatContext *s)
}
av_freep();
} else if (metadata_type == FLAC_METADATA_TYPE_PICTURE) {
-ret = ff_flac_parse_picture(s, buffer, metadata_size);
+ret = ff_flac_parse_picture(s, buffer, metadata_size, 1);
av_freep();
if (ret < 0) {
av_log(s, AV_LOG_ERROR, "Error parsing attached picture.\n");
diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
index 27d2c686b6..6f15204ada 100644
--- a/libavformat/oggparsevorbis.c
+++ b/libavformat/oggparsevorbis.c
@@ -165,7 +165,7 @@ int ff_vorbis_comment(AVFormatContext *as, AVDictionary **m,
av_freep();
av_freep();
if (ret > 0)
-ret = ff_flac_parse_picture(as, pict, ret);
+ret = ff_flac_parse_picture(as, pict, ret, 0);
Re: [FFmpeg-devel] [PATCH] libavformat/flacdec: Workaround for truncated metadata picture size
On Fri, Apr 24, 2020 at 11:26 AM Andreas Rheinhardt
wrote:
>
> Mattias Wadman:
> > On Fri, Apr 24, 2020 at 7:29 AM Andreas Rheinhardt
> > wrote:
> >>
> >> Mattias Wadman:
> >>> lavf flacenc could previously write truncated metadata picture size if
> >>> the picture data did not fit in 24 bits. Detect this by truncting the
> >>> size found inside the picture block and if it matches the block size
> >>> use it and read rest of picture data.
> >>>
> >>> Also only enable this workaround flac files and not ogg files with flac
> >>> METADATA_BLOCK_PICTURE comment.
> >>>
> >>> flacenc was fixed in e447a4d112bcfee10126c54eb4481fa8712957c8
> >>> before the fix a broken flac for reproduction could be generated with:
> >>> ffmpeg -f lavfi -i sine -f lavfi -i color=red:size=2400x2400 -map 0:0
> >>> -map 1:0 -c:v:0 bmp -disposition:1 attached_pic -t 1 test.flac
> >>>
> >>> Fixes ticket 6333
> >>> ---
> >>> libavformat/flac_picture.c | 27 ---
> >>> libavformat/flac_picture.h | 2 +-
> >>> libavformat/flacdec.c| 2 +-
> >>> libavformat/oggparsevorbis.c | 2 +-
> >>> 4 files changed, 27 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
> >>> index 81ddf80465..f59ec8843f 100644
> >>> --- a/libavformat/flac_picture.c
> >>> +++ b/libavformat/flac_picture.c
> >>> @@ -27,16 +27,17 @@
> >>> #include "id3v2.h"
> >>> #include "internal.h"
> >>>
> >>> -int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
> >>> +int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int
> >>> buf_size, int truncate_workaround)
> >>> {
> >>> const CodecMime *mime = ff_id3v2_mime_tags;
> >>> enum AVCodecID id = AV_CODEC_ID_NONE;
> >>> AVBufferRef *data = NULL;
> >>> -uint8_t mimetype[64], *desc = NULL;
> >>> +uint8_t mimetype[64], *desc = NULL, *picturebuf = NULL;
> >>> GetByteContext g;
> >>> AVStream *st;
> >>> int width, height, ret = 0;
> >>> -unsigned int len, type;
> >>> +unsigned int type;
> >>> +uint32_t len, left;
> >>>
> >>> if (buf_size < 34) {
> >>> av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> >>> short\n");
> >>> @@ -114,6 +115,25 @@ int ff_flac_parse_picture(AVFormatContext *s,
> >>> uint8_t *buf, int buf_size)
> >>>
> >>> /* picture data */
> >>> len = bytestream2_get_be32u();
> >>> +
> >>> +// Workaround lavf flacenc bug that allowed writing truncated
> >>> metadata picture block size if
> >>> +// picture size did not fit in 24 bits
> >>> +left = bytestream2_get_bytes_left();
> >>> +if (truncate_workaround && len > left && (len & 0xff) == left) {
> >>> +uint32_t lendiff;
> >>> +
> >>> +av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture
> >>> size from %d to %d\n", left, len);
> >>> +
> >>> +picturebuf = av_malloc(len);
> >>> +if (!picturebuf)
> >>> +RETURN_ERROR(AVERROR(ENOMEM));
> >>> +lendiff = len - left;
> >>> +bytestream2_get_buffer(, picturebuf, left);
> >>> +if (avio_read(s->pb, picturebuf+left, lendiff) < lendiff)
> >>> +RETURN_ERROR(AVERROR_INVALIDDATA);
> >>> +bytestream2_init(, picturebuf, len);
> >>> +}
> >>> +
> >>> if (len <= 0 || len > bytestream2_get_bytes_left()) {
> >>> av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> >>> short\n");
> >>> if (s->error_recognition & AV_EF_EXPLODE)
> >>> @@ -155,6 +175,7 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t
> >>> *buf, int buf_size)
> >>> fail:
> >>> av_buffer_unref();
> >>> av_freep();
> >>> +av_freep();
> >>
> >> I don't like that you are allocating a very big buffer and read data
> >> into this buffer just to copy it
Re: [FFmpeg-devel] [PATCH] libavformat/flacdec: Workaround for truncated metadata picture size
On Fri, Apr 24, 2020 at 7:29 AM Andreas Rheinhardt
wrote:
>
> Mattias Wadman:
> > lavf flacenc could previously write truncated metadata picture size if
> > the picture data did not fit in 24 bits. Detect this by truncting the
> > size found inside the picture block and if it matches the block size
> > use it and read rest of picture data.
> >
> > Also only enable this workaround flac files and not ogg files with flac
> > METADATA_BLOCK_PICTURE comment.
> >
> > flacenc was fixed in e447a4d112bcfee10126c54eb4481fa8712957c8
> > before the fix a broken flac for reproduction could be generated with:
> > ffmpeg -f lavfi -i sine -f lavfi -i color=red:size=2400x2400 -map 0:0 -map
> > 1:0 -c:v:0 bmp -disposition:1 attached_pic -t 1 test.flac
> >
> > Fixes ticket 6333
> > ---
> > libavformat/flac_picture.c | 27 ---
> > libavformat/flac_picture.h | 2 +-
> > libavformat/flacdec.c| 2 +-
> > libavformat/oggparsevorbis.c | 2 +-
> > 4 files changed, 27 insertions(+), 6 deletions(-)
> >
> > diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
> > index 81ddf80465..f59ec8843f 100644
> > --- a/libavformat/flac_picture.c
> > +++ b/libavformat/flac_picture.c
> > @@ -27,16 +27,17 @@
> > #include "id3v2.h"
> > #include "internal.h"
> >
> > -int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
> > +int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size,
> > int truncate_workaround)
> > {
> > const CodecMime *mime = ff_id3v2_mime_tags;
> > enum AVCodecID id = AV_CODEC_ID_NONE;
> > AVBufferRef *data = NULL;
> > -uint8_t mimetype[64], *desc = NULL;
> > +uint8_t mimetype[64], *desc = NULL, *picturebuf = NULL;
> > GetByteContext g;
> > AVStream *st;
> > int width, height, ret = 0;
> > -unsigned int len, type;
> > +unsigned int type;
> > +uint32_t len, left;
> >
> > if (buf_size < 34) {
> > av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> > short\n");
> > @@ -114,6 +115,25 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t
> > *buf, int buf_size)
> >
> > /* picture data */
> > len = bytestream2_get_be32u();
> > +
> > +// Workaround lavf flacenc bug that allowed writing truncated metadata
> > picture block size if
> > +// picture size did not fit in 24 bits
> > +left = bytestream2_get_bytes_left();
> > +if (truncate_workaround && len > left && (len & 0xff) == left) {
> > +uint32_t lendiff;
> > +
> > +av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture size
> > from %d to %d\n", left, len);
> > +
> > +picturebuf = av_malloc(len);
> > +if (!picturebuf)
> > +RETURN_ERROR(AVERROR(ENOMEM));
> > +lendiff = len - left;
> > +bytestream2_get_buffer(, picturebuf, left);
> > +if (avio_read(s->pb, picturebuf+left, lendiff) < lendiff)
> > +RETURN_ERROR(AVERROR_INVALIDDATA);
> > +bytestream2_init(, picturebuf, len);
> > +}
> > +
> > if (len <= 0 || len > bytestream2_get_bytes_left()) {
> > av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> > short\n");
> > if (s->error_recognition & AV_EF_EXPLODE)
> > @@ -155,6 +175,7 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t
> > *buf, int buf_size)
> > fail:
> > av_buffer_unref();
> > av_freep();
> > +av_freep();
>
> I don't like that you are allocating a very big buffer and read data
> into this buffer just to copy it into another buffer (that is not given
> in advance, but allocated by this function, too).
Ok thanks, yeap not so nice. Guess there are some alternatives i come up with:
1) Probe for the issue in flac_read_header and if detected realloc and
read more then call unmodified ff_flac_parse_picture
2) Make ff_flac_parse_picture able to say it needs more data
3) Make ff_flac_parse_picture able to realloc the passed buf, so
change buf arg to uint8_t **buf
-Mattias
>
> - Andreas
>
> >
> > return ret;
> > }
> > diff --git a/libavformat/flac_picture.h b/libavformat/flac_picture.h
> > index 4374b6f4f6..61fd0c8806 100644
> > --- a/libavformat/flac_picture.h
> > +++ b/libavformat/flac_picture.h
> > @@ -26,6 +26,6 @@
> >
> > #
Re: [FFmpeg-devel] [PATCH] libavformat/flacdec: Workaround for truncated metadata picture size
On Thu, Apr 23, 2020 at 5:15 PM Mattias Wadman wrote:
>
> lavf flacenc could previously write truncated metadata picture size if
> the picture data did not fit in 24 bits. Detect this by truncting the
> size found inside the picture block and if it matches the block size
> use it and read rest of picture data.
>
> Also only enable this workaround flac files and not ogg files with flac
> METADATA_BLOCK_PICTURE comment.
>
> flacenc was fixed in e447a4d112bcfee10126c54eb4481fa8712957c8
> before the fix a broken flac for reproduction could be generated with:
> ffmpeg -f lavfi -i sine -f lavfi -i color=red:size=2400x2400 -map 0:0 -map
> 1:0 -c:v:0 bmp -disposition:1 attached_pic -t 1 test.flac
>
> Fixes ticket 6333
> ---
> libavformat/flac_picture.c | 27 ---
> libavformat/flac_picture.h | 2 +-
> libavformat/flacdec.c| 2 +-
> libavformat/oggparsevorbis.c | 2 +-
> 4 files changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
> index 81ddf80465..f59ec8843f 100644
> --- a/libavformat/flac_picture.c
> +++ b/libavformat/flac_picture.c
> @@ -27,16 +27,17 @@
> #include "id3v2.h"
> #include "internal.h"
>
> -int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
> +int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size,
> int truncate_workaround)
> {
> const CodecMime *mime = ff_id3v2_mime_tags;
> enum AVCodecID id = AV_CODEC_ID_NONE;
> AVBufferRef *data = NULL;
> -uint8_t mimetype[64], *desc = NULL;
> +uint8_t mimetype[64], *desc = NULL, *picturebuf = NULL;
> GetByteContext g;
> AVStream *st;
> int width, height, ret = 0;
> -unsigned int len, type;
> +unsigned int type;
> +uint32_t len, left;
>
> if (buf_size < 34) {
> av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> short\n");
> @@ -114,6 +115,25 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t
> *buf, int buf_size)
>
> /* picture data */
> len = bytestream2_get_be32u();
> +
> +// Workaround lavf flacenc bug that allowed writing truncated metadata
> picture block size if
> +// picture size did not fit in 24 bits
> +left = bytestream2_get_bytes_left();
> +if (truncate_workaround && len > left && (len & 0xff) == left) {
> +uint32_t lendiff;
> +
> +av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture size
> from %d to %d\n", left, len);
> +
> +picturebuf = av_malloc(len);
> +if (!picturebuf)
> +RETURN_ERROR(AVERROR(ENOMEM));
> +lendiff = len - left;
> +bytestream2_get_buffer(, picturebuf, left);
> +if (avio_read(s->pb, picturebuf+left, lendiff) < lendiff)
> +RETURN_ERROR(AVERROR_INVALIDDATA);
> +bytestream2_init(, picturebuf, len);
> +}
> +
> if (len <= 0 || len > bytestream2_get_bytes_left()) {
> av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> short\n");
> if (s->error_recognition & AV_EF_EXPLODE)
> @@ -155,6 +175,7 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t
> *buf, int buf_size)
> fail:
> av_buffer_unref();
> av_freep();
> +av_freep();
>
> return ret;
> }
> diff --git a/libavformat/flac_picture.h b/libavformat/flac_picture.h
> index 4374b6f4f6..61fd0c8806 100644
> --- a/libavformat/flac_picture.h
> +++ b/libavformat/flac_picture.h
> @@ -26,6 +26,6 @@
>
> #define RETURN_ERROR(code) do { ret = (code); goto fail; } while (0)
>
> -int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size);
> +int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size,
> int truncate_workaround);
>
> #endif /* AVFORMAT_FLAC_PICTURE_H */
> diff --git a/libavformat/flacdec.c b/libavformat/flacdec.c
> index cb516fb1f3..79c05f14bf 100644
> --- a/libavformat/flacdec.c
> +++ b/libavformat/flacdec.c
> @@ -146,7 +146,7 @@ static int flac_read_header(AVFormatContext *s)
> }
> av_freep();
> } else if (metadata_type == FLAC_METADATA_TYPE_PICTURE) {
> -ret = ff_flac_parse_picture(s, buffer, metadata_size);
> +ret = ff_flac_parse_picture(s, buffer, metadata_size, 1);
> av_freep();
> if (ret < 0) {
> av_log(s, AV_LOG_ERROR, "Error parsing attached picture.\n");
> diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
> index 27d2c686b6..6f15204ada 100
[FFmpeg-devel] [PATCH] libavformat/flacdec: Workaround for truncated metadata picture size
lavf flacenc could previously write truncated metadata picture size if
the picture data did not fit in 24 bits. Detect this by truncting the
size found inside the picture block and if it matches the block size
use it and read rest of picture data.
Also only enable this workaround flac files and not ogg files with flac
METADATA_BLOCK_PICTURE comment.
flacenc was fixed in e447a4d112bcfee10126c54eb4481fa8712957c8
before the fix a broken flac for reproduction could be generated with:
ffmpeg -f lavfi -i sine -f lavfi -i color=red:size=2400x2400 -map 0:0 -map 1:0
-c:v:0 bmp -disposition:1 attached_pic -t 1 test.flac
Fixes ticket 6333
---
libavformat/flac_picture.c | 27 ---
libavformat/flac_picture.h | 2 +-
libavformat/flacdec.c| 2 +-
libavformat/oggparsevorbis.c | 2 +-
4 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
index 81ddf80465..f59ec8843f 100644
--- a/libavformat/flac_picture.c
+++ b/libavformat/flac_picture.c
@@ -27,16 +27,17 @@
#include "id3v2.h"
#include "internal.h"
-int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
+int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size, int
truncate_workaround)
{
const CodecMime *mime = ff_id3v2_mime_tags;
enum AVCodecID id = AV_CODEC_ID_NONE;
AVBufferRef *data = NULL;
-uint8_t mimetype[64], *desc = NULL;
+uint8_t mimetype[64], *desc = NULL, *picturebuf = NULL;
GetByteContext g;
AVStream *st;
int width, height, ret = 0;
-unsigned int len, type;
+unsigned int type;
+uint32_t len, left;
if (buf_size < 34) {
av_log(s, AV_LOG_ERROR, "Attached picture metadata block too short\n");
@@ -114,6 +115,25 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t
*buf, int buf_size)
/* picture data */
len = bytestream2_get_be32u();
+
+// Workaround lavf flacenc bug that allowed writing truncated metadata
picture block size if
+// picture size did not fit in 24 bits
+left = bytestream2_get_bytes_left();
+if (truncate_workaround && len > left && (len & 0xff) == left) {
+uint32_t lendiff;
+
+av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture size
from %d to %d\n", left, len);
+
+picturebuf = av_malloc(len);
+if (!picturebuf)
+RETURN_ERROR(AVERROR(ENOMEM));
+lendiff = len - left;
+bytestream2_get_buffer(, picturebuf, left);
+if (avio_read(s->pb, picturebuf+left, lendiff) < lendiff)
+RETURN_ERROR(AVERROR_INVALIDDATA);
+bytestream2_init(, picturebuf, len);
+}
+
if (len <= 0 || len > bytestream2_get_bytes_left()) {
av_log(s, AV_LOG_ERROR, "Attached picture metadata block too short\n");
if (s->error_recognition & AV_EF_EXPLODE)
@@ -155,6 +175,7 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf,
int buf_size)
fail:
av_buffer_unref();
av_freep();
+av_freep();
return ret;
}
diff --git a/libavformat/flac_picture.h b/libavformat/flac_picture.h
index 4374b6f4f6..61fd0c8806 100644
--- a/libavformat/flac_picture.h
+++ b/libavformat/flac_picture.h
@@ -26,6 +26,6 @@
#define RETURN_ERROR(code) do { ret = (code); goto fail; } while (0)
-int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size);
+int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size, int
truncate_workaround);
#endif /* AVFORMAT_FLAC_PICTURE_H */
diff --git a/libavformat/flacdec.c b/libavformat/flacdec.c
index cb516fb1f3..79c05f14bf 100644
--- a/libavformat/flacdec.c
+++ b/libavformat/flacdec.c
@@ -146,7 +146,7 @@ static int flac_read_header(AVFormatContext *s)
}
av_freep();
} else if (metadata_type == FLAC_METADATA_TYPE_PICTURE) {
-ret = ff_flac_parse_picture(s, buffer, metadata_size);
+ret = ff_flac_parse_picture(s, buffer, metadata_size, 1);
av_freep();
if (ret < 0) {
av_log(s, AV_LOG_ERROR, "Error parsing attached picture.\n");
diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
index 27d2c686b6..6f15204ada 100644
--- a/libavformat/oggparsevorbis.c
+++ b/libavformat/oggparsevorbis.c
@@ -165,7 +165,7 @@ int ff_vorbis_comment(AVFormatContext *as, AVDictionary **m,
av_freep();
av_freep();
if (ret > 0)
-ret = ff_flac_parse_picture(as, pict, ret);
+ret = ff_flac_parse_picture(as, pict, ret, 0);
av_freep();
if (ret < 0) {
av_log(as, AV_LOG_WARNING, "Failed to parse cover art
block.\n");
--
2.18.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
Re: [FFmpeg-devel] [PATCH] libavformat/flacdec: Workaround for truncated metadata picture size
On Wed, Apr 22, 2020 at 5:18 PM Andreas Rheinhardt
wrote:
>
> Mattias Wadman:
> > lavf flacenc could previously write truncated metadata picture size if
> > the picture data did not fit in 24 bits. Detect this by truncting the
> > size found inside the picture block and if it matches the block size
> > use it and read rest of picture data.
> >
> > flacenc was fixed in e447a4d112bcfee10126c54eb4481fa8712957c8
> > ---
> > libavformat/flac_picture.c | 23 +--
> > 1 file changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
> > index 81ddf80465..17be497f95 100644
> > --- a/libavformat/flac_picture.c
> > +++ b/libavformat/flac_picture.c
> > @@ -32,11 +32,12 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t
> > *buf, int buf_size)
> > const CodecMime *mime = ff_id3v2_mime_tags;
> > enum AVCodecID id = AV_CODEC_ID_NONE;
> > AVBufferRef *data = NULL;
> > -uint8_t mimetype[64], *desc = NULL;
> > +uint8_t mimetype[64], *desc = NULL, *picturebuf = NULL;
> > GetByteContext g;
> > AVStream *st;
> > int width, height, ret = 0;
> > -unsigned int len, type;
> > +unsigned int type;
> > +uint32_t len, left, lendiff;
> >
> > if (buf_size < 34) {
> > av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> > short\n");
> > @@ -114,6 +115,23 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t
> > *buf, int buf_size)
> >
> > /* picture data */
> > len = bytestream2_get_be32u();
> > +
> > +// Workaround lavf flacenc bug that allowed writing truncated metadata
> > picture block size if
> > +// picture size did not fit in 24 bits
> > +left = bytestream2_get_bytes_left();
> > +if (len > left && (len & 0xff) == left) {
> > +av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture size
> > from %d to %d\n", left, len);
> > +
> > +picturebuf = av_malloc(len);
> > +if (!picturebuf)
> > +RETURN_ERROR(AVERROR(ENOMEM));
> > +lendiff = len - left;
> > +bytestream2_get_buffer(, picturebuf, left);
> > +if (avio_read(s->pb, picturebuf+left, lendiff) < lendiff)
> > +RETURN_ERROR(AVERROR_INVALIDDATA);
> > +bytestream2_init(, picturebuf, len);
> > +}
> > +
> > if (len <= 0 || len > bytestream2_get_bytes_left()) {
> > av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> > short\n");
> > if (s->error_recognition & AV_EF_EXPLODE)
> > @@ -155,6 +173,7 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t
> > *buf, int buf_size)
> > fail:
> > av_buffer_unref();
> > av_freep();
> > +av_freep();
> >
> > return ret;
> > }
> >
> 1. This fixes ticket 6333 (you will now no longer run into the flac
> parser bug), doesn't it? If so, you should mention it.
Yes this patch should fixat that. Didn't know there was a ticket, will
mention it.
> 2. ff_flac_parse_picture() is called from two places: The flac demuxer
> as well as the vorbis-in-ogg demuxer. In the latter case the picture
> data is base64-encoded and reading anything via the AVIOContext is a
> very bad idea (ff_vorbis_comment() is for e.g. used to parse
> VorbisComments contained in the CodecPrivate of FLAC tracks (for channel
> masks) and if such a CodecPrivate also contained an embedded picture
> that happens to trigger this scenario, you would read more data
> believing it comes from immediately after the buffer you have received,
> but that is just not true). You need to add a parameter to distinguish
> these cases.
Ok will look into that. Thanks for the speedy review!
-Mattias
>
> - Andreas
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH] libavformat/flacdec: Workaround for truncated metadata picture size
lavf flacenc could previously write truncated metadata picture size if
the picture data did not fit in 24 bits. Detect this by truncting the
size found inside the picture block and if it matches the block size
use it and read rest of picture data.
flacenc was fixed in e447a4d112bcfee10126c54eb4481fa8712957c8
---
libavformat/flac_picture.c | 23 +--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
index 81ddf80465..17be497f95 100644
--- a/libavformat/flac_picture.c
+++ b/libavformat/flac_picture.c
@@ -32,11 +32,12 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf,
int buf_size)
const CodecMime *mime = ff_id3v2_mime_tags;
enum AVCodecID id = AV_CODEC_ID_NONE;
AVBufferRef *data = NULL;
-uint8_t mimetype[64], *desc = NULL;
+uint8_t mimetype[64], *desc = NULL, *picturebuf = NULL;
GetByteContext g;
AVStream *st;
int width, height, ret = 0;
-unsigned int len, type;
+unsigned int type;
+uint32_t len, left, lendiff;
if (buf_size < 34) {
av_log(s, AV_LOG_ERROR, "Attached picture metadata block too short\n");
@@ -114,6 +115,23 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t
*buf, int buf_size)
/* picture data */
len = bytestream2_get_be32u();
+
+// Workaround lavf flacenc bug that allowed writing truncated metadata
picture block size if
+// picture size did not fit in 24 bits
+left = bytestream2_get_bytes_left();
+if (len > left && (len & 0xff) == left) {
+av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture size
from %d to %d\n", left, len);
+
+picturebuf = av_malloc(len);
+if (!picturebuf)
+RETURN_ERROR(AVERROR(ENOMEM));
+lendiff = len - left;
+bytestream2_get_buffer(, picturebuf, left);
+if (avio_read(s->pb, picturebuf+left, lendiff) < lendiff)
+RETURN_ERROR(AVERROR_INVALIDDATA);
+bytestream2_init(, picturebuf, len);
+}
+
if (len <= 0 || len > bytestream2_get_bytes_left()) {
av_log(s, AV_LOG_ERROR, "Attached picture metadata block too short\n");
if (s->error_recognition & AV_EF_EXPLODE)
@@ -155,6 +173,7 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf,
int buf_size)
fail:
av_buffer_unref();
av_freep();
+av_freep();
return ret;
}
--
2.18.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] libavformat/oggparsevorbis: Use case-insensitive key compare for vorbis picture
Thanks!
Strange, bash history says:
git format-patch --to=ffmpeg-devel@ffmpeg.org HEAD~..HEAD
git send-email *.patch
But i do have this gitconfig:
[diff]
noprefix = true
Seems format-patch uses it That was unexpected, removing that config.
-Mattias
On Sat, Apr 11, 2020 at 12:59 PM Carl Eugen Hoyos
wrote:
> Am Sa., 11. Apr. 2020 um 11:53 Uhr schrieb Mattias Wadman
> :
> >
> > Regression since 8d3630c5402fdda2889fe4f74f7dcdd50ebca654 where keys
> were changed
> > to not be touppered but the picture block strcmp was not changed to be
> case-insensitive.
> > ---
> > libavformat/oggparsevorbis.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git libavformat/oggparsevorbis.c libavformat/oggparsevorbis.c
> > index 8dd27e7770..27d2c686b6 100644
> > --- libavformat/oggparsevorbis.c
> > +++ libavformat/oggparsevorbis.c
> > @@ -151,7 +151,7 @@ int ff_vorbis_comment(AVFormatContext *as,
> AVDictionary **m,
> > * 'METADATA_BLOCK_PICTURE'. This is the preferred and
> > * recommended way of embedding cover art within
> VorbisComments."
> > */
> > -if (!strcmp(tt, "METADATA_BLOCK_PICTURE") && parse_picture)
> {
> > +if (!av_strcasecmp(tt, "METADATA_BLOCK_PICTURE") &&
> parse_picture) {
> > int ret, len = AV_BASE64_DECODE_SIZE(vl);
> > char *pict = av_malloc(len);
>
> For future patches:
> I have no idea how you created this but my git didn't like it.
>
> Patch applied, thank you!
>
> Carl Eugen
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH] libavformat/oggparsevorbis: Use case-insensitive key compare for vorbis picture
Regression since 8d3630c5402fdda2889fe4f74f7dcdd50ebca654 where keys were
changed
to not be touppered but the picture block strcmp was not changed to be
case-insensitive.
---
libavformat/oggparsevorbis.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git libavformat/oggparsevorbis.c libavformat/oggparsevorbis.c
index 8dd27e7770..27d2c686b6 100644
--- libavformat/oggparsevorbis.c
+++ libavformat/oggparsevorbis.c
@@ -151,7 +151,7 @@ int ff_vorbis_comment(AVFormatContext *as, AVDictionary **m,
* 'METADATA_BLOCK_PICTURE'. This is the preferred and
* recommended way of embedding cover art within VorbisComments."
*/
-if (!strcmp(tt, "METADATA_BLOCK_PICTURE") && parse_picture) {
+if (!av_strcasecmp(tt, "METADATA_BLOCK_PICTURE") && parse_picture)
{
int ret, len = AV_BASE64_DECODE_SIZE(vl);
char *pict = av_malloc(len);
--
2.22.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH V2] libavformat/flacenc: reject too big picture blocks
Sorry i failed to get gmail to play nice with patches :( sent a new message using git send-email, hope that works. On Wed, Oct 30, 2019 at 12:51 PM Michael Niedermayer wrote: > > On Tue, Oct 29, 2019 at 02:42:47PM +0100, Mattias Wadman wrote: > > A too big picture will case the muxer to write a truncated block size > > (uint24) > > causing the output file to be corrupt. > > > > How to reproduce: > > > > Write a file with truncated block size: > > ffmpeg -y -f lavfi -i sine -f lavfi -i color=red:size=2400x2400 -map > > 0:a:0 -map 1:v:0 -c:v:0 bmp -disposition:1 attached_pic -t 1 test.flac > > > > Try to decode: > > ffmpeg -i test.flac test.wav > > > > Signed-off-by: Mattias Wadman > > --- > > libavformat/flacenc.c | 10 -- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/libavformat/flacenc.c b/libavformat/flacenc.c > > index 93cc79bbe0..7b51c11404 100644 > > --- a/libavformat/flacenc.c > > +++ b/libavformat/flacenc.c > > @@ -93,7 +93,7 @@ static int flac_write_picture(struct AVFormatContext > > *s, AVPacket *pkt) > > AVDictionaryEntry *e; > > const char *mimetype = NULL, *desc = ""; > > const AVStream *st = s->streams[pkt->stream_index]; > > -int i, mimelen, desclen, type = 0; > > +int i, mimelen, desclen, type = 0, blocklen; > > > > if (!pkt->data) > > return 0; > > @@ -140,8 +140,14 @@ static int flac_write_picture(struct > > AVFormatContext *s, AVPacket *pkt) > > desc = e->value; > > Applying: libavformat/flacenc: reject too big picture blocks > error: corrupt patch at line 10 > > probably line/word wrap > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > If you fake or manipulate statistics in a paper in physics you will never > get a job again. > If you fake or manipulate statistics in a paper in medicin you will get > a job for life at the pharma industry. > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 1/1] libavformat/flacenc: reject too big picture blocks
A too big picture will case the muxer to write a truncated block size (uint24)
causing the output file to be corrupt.
How to reproduce:
Write a file with truncated block size:
ffmpeg -y -f lavfi -i sine -f lavfi -i color=red:size=2400x2400 -map 0:a:0 -map
1:v:0 -c:v:0 bmp -disposition:1 attached_pic -t 1 test.flac
Try to decode:
ffmpeg -i test.flac test.wav
Signed-off-by: Mattias Wadman
---
libavformat/flacenc.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/libavformat/flacenc.c b/libavformat/flacenc.c
index 93cc79bbe0..7b51c11404 100644
--- a/libavformat/flacenc.c
+++ b/libavformat/flacenc.c
@@ -93,7 +93,7 @@ static int flac_write_picture(struct AVFormatContext *s,
AVPacket *pkt)
AVDictionaryEntry *e;
const char *mimetype = NULL, *desc = "";
const AVStream *st = s->streams[pkt->stream_index];
-int i, mimelen, desclen, type = 0;
+int i, mimelen, desclen, type = 0, blocklen;
if (!pkt->data)
return 0;
@@ -140,8 +140,14 @@ static int flac_write_picture(struct AVFormatContext *s,
AVPacket *pkt)
desc = e->value;
desclen = strlen(desc);
+blocklen = 4 + 4 + mimelen + 4 + desclen + 4 + 4 + 4 + 4 + 4 + pkt->size;
+if (blocklen >= 1<<24) {
+ av_log(s, AV_LOG_ERROR, "Picture block too big %d >= %d\n", blocklen,
1<<24);
+return AVERROR(EINVAL);
+}
+
avio_w8(pb, 0x06);
-avio_wb24(pb, 4 + 4 + mimelen + 4 + desclen + 4 + 4 + 4 + 4 + 4 +
pkt->size);
+avio_wb24(pb, blocklen);
avio_wb32(pb, type);
--
2.18.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 0/1] libavformat/flacenc: reject too big picture blocks
Mattias Wadman (1): libavformat/flacenc: reject too big picture blocks libavformat/flacenc.c | 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) -- 2.18.0 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH V2] libavformat/flacenc: reject too big picture blocks
A too big picture will case the muxer to write a truncated block size (uint24)
causing the output file to be corrupt.
How to reproduce:
Write a file with truncated block size:
ffmpeg -y -f lavfi -i sine -f lavfi -i color=red:size=2400x2400 -map
0:a:0 -map 1:v:0 -c:v:0 bmp -disposition:1 attached_pic -t 1 test.flac
Try to decode:
ffmpeg -i test.flac test.wav
Signed-off-by: Mattias Wadman
---
libavformat/flacenc.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/libavformat/flacenc.c b/libavformat/flacenc.c
index 93cc79bbe0..7b51c11404 100644
--- a/libavformat/flacenc.c
+++ b/libavformat/flacenc.c
@@ -93,7 +93,7 @@ static int flac_write_picture(struct AVFormatContext
*s, AVPacket *pkt)
AVDictionaryEntry *e;
const char *mimetype = NULL, *desc = "";
const AVStream *st = s->streams[pkt->stream_index];
-int i, mimelen, desclen, type = 0;
+int i, mimelen, desclen, type = 0, blocklen;
if (!pkt->data)
return 0;
@@ -140,8 +140,14 @@ static int flac_write_picture(struct
AVFormatContext *s, AVPacket *pkt)
desc = e->value;
desclen = strlen(desc);
+blocklen = 4 + 4 + mimelen + 4 + desclen + 4 + 4 + 4 + 4 + 4 + pkt->size;
+if (blocklen >= 1<<24) {
+ av_log(s, AV_LOG_ERROR, "Picture block too big %d >= %d\n",
blocklen, 1<<24);
+return AVERROR(EINVAL);
+}
+
avio_w8(pb, 0x06);
-avio_wb24(pb, 4 + 4 + mimelen + 4 + desclen + 4 + 4 + 4 + 4 + 4 +
pkt->size);
+avio_wb24(pb, blocklen);
avio_wb32(pb, type);
--
2.18.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] libavformat/flacenc: reject too big picture blocks
Think i messed up the formatting of the in-line patch somehow. Ill send the
patch as an attachment instead. Hope reply and attach is ok?
On Sun, Oct 27, 2019 at 8:22 PM Mattias Wadman
wrote:
> A too big picture will case the muxer to write a truncated block size
> (uint24)
> causing the output file to be corrupt.
>
> How to reproduce:
>
> Write a file with truncated block size:
> ffmpeg -y -f lavfi -i sine -f lavfi -i color=red:size=2400x2400 -map 0:a:0
> -map 1:v:0 -c:v:0 bmp -disposition:1 attached_pic -t 1 test.flac
>
> Try to decode:
> ffmpeg -i test.flac test.wav
>
> Signed-off-by: Mattias Wadman
> ---
> libavformat/flacenc.c | 10 --
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git libavformat/flacenc.c libavformat/flacenc.c
> index 93cc79bbe0..957bcb1123 100644
> --- libavformat/flacenc.c
> +++ libavformat/flacenc.c
> @@ -93,7 +93,7 @@ static int flac_write_picture(struct AVFormatContext *s,
> AVPacket *pkt)
> AVDictionaryEntry *e;
> const char *mimetype = NULL, *desc = "";
> const AVStream *st = s->streams[pkt->stream_index];
> -int i, mimelen, desclen, type = 0;
> +int i, mimelen, desclen, type = 0, blocklen = 0;
>
> if (!pkt->data)
> return 0;
> @@ -140,8 +140,14 @@ static int flac_write_picture(struct AVFormatContext
> *s, AVPacket *pkt)
> desc = e->value;
> desclen = strlen(desc);
>
> +blocklen = 4 + 4 + mimelen + 4 + desclen + 4 + 4 + 4 + 4 + 4 +
> pkt->size;
> +if (blocklen >= 1<<24) {
> + av_log(s, AV_LOG_ERROR, "Picture block too big %d >= %d\n",
> blocklen, 1<<24);
> +return AVERROR(EINVAL);
> +}
> +
> avio_w8(pb, 0x06);
> -avio_wb24(pb, 4 + 4 + mimelen + 4 + desclen + 4 + 4 + 4 + 4 + 4 +
> pkt->size);
> +avio_wb24(pb, blocklen);
>
> avio_wb32(pb, type);
>
> --
> 2.22.0
>
0001-libavformat-flacenc-reject-too-large-picture-blocks.patch
Description: Binary data
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH] libavformat/flacenc: reject too big picture blocks
A too big picture will case the muxer to write a truncated block size
(uint24)
causing the output file to be corrupt.
How to reproduce:
Write a file with truncated block size:
ffmpeg -y -f lavfi -i sine -f lavfi -i color=red:size=2400x2400 -map 0:a:0
-map 1:v:0 -c:v:0 bmp -disposition:1 attached_pic -t 1 test.flac
Try to decode:
ffmpeg -i test.flac test.wav
Signed-off-by: Mattias Wadman
---
libavformat/flacenc.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git libavformat/flacenc.c libavformat/flacenc.c
index 93cc79bbe0..957bcb1123 100644
--- libavformat/flacenc.c
+++ libavformat/flacenc.c
@@ -93,7 +93,7 @@ static int flac_write_picture(struct AVFormatContext *s,
AVPacket *pkt)
AVDictionaryEntry *e;
const char *mimetype = NULL, *desc = "";
const AVStream *st = s->streams[pkt->stream_index];
-int i, mimelen, desclen, type = 0;
+int i, mimelen, desclen, type = 0, blocklen = 0;
if (!pkt->data)
return 0;
@@ -140,8 +140,14 @@ static int flac_write_picture(struct AVFormatContext
*s, AVPacket *pkt)
desc = e->value;
desclen = strlen(desc);
+blocklen = 4 + 4 + mimelen + 4 + desclen + 4 + 4 + 4 + 4 + 4 +
pkt->size;
+if (blocklen >= 1<<24) {
+ av_log(s, AV_LOG_ERROR, "Picture block too big %d >= %d\n",
blocklen, 1<<24);
+return AVERROR(EINVAL);
+}
+
avio_w8(pb, 0x06);
-avio_wb24(pb, 4 + 4 + mimelen + 4 + desclen + 4 + 4 + 4 + 4 + 4 +
pkt->size);
+avio_wb24(pb, blocklen);
avio_wb32(pb, type);
--
2.22.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
