From patchwork Mon May 18 10:35:28 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mattias Wadman X-Patchwork-Id: 19736 Return-Path: X-Original-To: patchwork@ffaux-bg.ffmpeg.org Delivered-To: patchwork@ffaux-bg.ffmpeg.org Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by ffaux.localdomain (Postfix) with ESMTP id D31B144AAB9 for ; Mon, 18 May 2020 13:35:54 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id A1E0668AA64; Mon, 18 May 2020 13:35:54 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wr1-f52.google.com (mail-wr1-f52.google.com [209.85.221.52]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 7F553680353 for ; Mon, 18 May 2020 13:35:48 +0300 (EEST) Received: by mail-wr1-f52.google.com with SMTP id 50so11171922wrc.11 for ; Mon, 18 May 2020 03:35:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id; bh=P04zhFg+mH1XHkesh1/LFmGL2T7Zxgx+ZHpZ9N+Exuw=; b=TJAyuFVUmW4xHY85LLqBaoIxp9vhvZG3uHNXdKRhlR6JzQ97ShanYUvdPWc+z63+d+ B7wHDZ3WZjqgDb+/lxS1q6FuCl51CeSN0vqOxVVYTXbUiGTeJxsnWWyBu3q969zWp82g 15K/NeVqqv1UCbb22hVBKd4A3+FYrBOrFaBtvy3oIFHIdkYG/cS2rpSLjwovSI83y1mS XwTATfKzUSq3DROH7dE5BZ0PDDfSsC5OZ0ORI8kTnRh7aBNcI6wfSOgPknfyJzwflKzL pIq3m++r3pcd9KaUWR5pjJJhebE2aW94u6dE6W1AAa5azniDQXT6LQSqMfi0zRKflS22 qiBQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=P04zhFg+mH1XHkesh1/LFmGL2T7Zxgx+ZHpZ9N+Exuw=; b=HmYRX8Mq2Cqd9ahpzCRmvWm07bq9/EY/qJPCvXaP5jaBbFf5xLIbYQfS5RKgBejqya 3AeEP1pxooUU3LeB+yPBn6qiFpBV+6DuII8Tqs0UR7/u/BAJk26jPwc43BvNP5LZY0ih mf2JXBr8KwYb8Inz0Y4UMOwnfrbx+jja96cdvAp/XLH6kza3NTqQNKW+xKQPn6R29qE1 L2P860Lksx7YEvpa813saQwTWqbuBsIKszdIboMyJIIqwMWrCVki/1bgMLrlcatc3HPe jr41R40DzVMvzgYT3+N92JWLIp0vWTKUNiB+L/S1K+oftWXVloIAuQgN+hyZ5Npg5g3n wpkw== X-Gm-Message-State: AOAM531kbxiyptCbRmQCfmLixmu1G6YpK4p+Z6dl8oSLvsgSbwx249qG wLLLJRHCEK67nq3Gl0ATxjQ0qlI1qJw= X-Google-Smtp-Source: ABdhPJxnXpNqNYo3DV3S9iRQjCLDwL1W3w4Ua5UpzHCkg18r4uWdRXLpxpcMqeFJvc8Z0m+u6Pr3tw== X-Received: by 2002:adf:fb0f:: with SMTP id c15mr20210674wrr.410.1589798147372; Mon, 18 May 2020 03:35:47 -0700 (PDT) Received: from work-2.local.net (c-388de253.016-130-73746f46.bbcust.telenor.se. [83.226.141.56]) by smtp.gmail.com with ESMTPSA id w15sm15395144wmi.35.2020.05.18.03.35.46 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Mon, 18 May 2020 03:35:46 -0700 (PDT) From: Mattias Wadman To: ffmpeg-devel@ffmpeg.org Date: Mon, 18 May 2020 12:35:28 +0200 Message-Id: <20200518103528.59425-1-mattias.wadman@gmail.com> X-Mailer: git-send-email 2.18.0 Subject: [FFmpeg-devel] [PATCH v4] libavformat/flacdec: Workaround for truncated metadata picture size X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Cc: Mattias Wadman MIME-Version: 1.0 Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" 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(&g); - if (len <= 0 || len > bytestream2_get_bytes_left(&g)) { - 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(&g); + 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 & 0xffffff) == 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(&g, data->data, len); + + if (trunclen == 0) { + bytestream2_get_bufferu(&g, 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(&g, 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(&buffer); } 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(&buffer); 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(&tt); av_freep(&ct); if (ret > 0) - ret = ff_flac_parse_picture(as, pict, ret); + ret = ff_flac_parse_picture(as, pict, ret, 0); av_freep(&pict); if (ret < 0) { av_log(as, AV_LOG_WARNING, "Failed to parse cover art block.\n");