From patchwork Sun May 3 10:09:45 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 19450 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 00C1944A071 for ; Sun, 3 May 2020 13:10:00 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id C8FD668BD86; Sun, 3 May 2020 13:09:59 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wr1-f53.google.com (mail-wr1-f53.google.com [209.85.221.53]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 9D49668BD3B for ; Sun, 3 May 2020 13:09:53 +0300 (EEST) Received: by mail-wr1-f53.google.com with SMTP id o27so11989436wra.12 for ; Sun, 03 May 2020 03:09:53 -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:mime-version :content-transfer-encoding; bh=WNlR8KYg9JmQWu29CtCKI2kLVXQbS6fGc7qWve+GAfU=; b=hEtUmTbAQeF1SffLbQTAlvY6cr3pJW7S8HAdFbQkZYP7J0pVqiDDzwg1EiUkNQgOHs xcDfoHHifPa+6WGHMj3gsq2+RxSWfsCMNyKF2F1GX0KGDeV53CWf0Kvxcaow2zBGvLUW X9za0Wa1dKwmZ/74n3hDuOkV1ie2SuWRm6+BiWZhG/ds//j2UjUxc6U/0rumcpZrniw7 oMHSmnX+Gv6Aph/LaPlM2F3Ed5bpOphSSaqPTyRUizH4lxhvYFoRAy/FwYqQ/lx/Wvmo 60cJoSw+oxIIbZxwwGf+60ntamAds57HiOOamOpkskyLIvB1um2EZnr5mPbHPdksHrjI KtTg== 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:mime-version :content-transfer-encoding; bh=WNlR8KYg9JmQWu29CtCKI2kLVXQbS6fGc7qWve+GAfU=; b=RbCvgalYyHa+gsFnG+8nAMKMVI72/N9gtlz1+x4M7qTE2sQ9grcHVdCfQm97yxHX3e zl+p+ZgFhvngadAk1u6bJvvZIwmzUBnQ/pjKCtvt9MscgIXOVqYMKaPC5tlvF6ChUH2o JxZS89/RNXRv9egNpEsulTy3AjW7bOp0Fd5OxlKhti7r4kDMR0LTMxrfSIvOpq8yz6Cs rnGgpNyYe935F64YSyODOIw+vX6h1Kt7UR4W2XEzdKzMX0U+ChBZjBqmY85x9rYO70R4 FW46CfcfvcpqmrH3z+pRd+aH+O6HnpvA6y/+5GO7EqUbZ6PWxWFt0yAj1Z8RadqLbOTx XgLg== X-Gm-Message-State: AGi0PuaUh/L32S0bT/9WpYePPJPqK8AaCGDbJxntEDA7SSacIRn298zD ho1gNmm5WNH+YMgwqXzyg2OAMlui X-Google-Smtp-Source: APiQypIO6H0XpkpevPVtmWVmD4OHaxEYcOUyJ2NEFljItZrn3VlhLIHw0C4V/SMuZuMIhU+e64UNQA== X-Received: by 2002:a5d:5189:: with SMTP id k9mr13099733wrv.3.1588500592349; Sun, 03 May 2020 03:09:52 -0700 (PDT) Received: from sblaptop.fritz.box (ipbcc1ab57.dynamic.kabel-deutschland.de. [188.193.171.87]) by smtp.gmail.com with ESMTPSA id c190sm8384568wme.10.2020.05.03.03.09.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 03 May 2020 03:09:51 -0700 (PDT) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Sun, 3 May 2020 12:09:45 +0200 Message-Id: <20200503100945.7226-1-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.20.1 MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH] avformat/matroskadec: Free right buffer on error 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: Andreas Rheinhardt Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Since commit 979b5b89594c7628bd846c63198cb64ef9d81d16, reverting the Matroska ContentCompression is no longer done inside matroska_parse_frame() (the function that creates AVPackets out of the parsed data (unless we are dealing with certain codecs that need special handling)), but instead in matroska_parse_block(). As a consequence, the data that matroska_parse_frame() receives is no longer always owned by an AVBuffer; it is owned by an AVBuffer iff no ContentCompression needed to be reversed; otherwise the data is independently allocated and needs to be freed on error. Whether the data is owned by an AVBuffer or not is indicated by a variable buf of type AVBufferRef *: If it is NULL, the data is independently allocated, if not it is owned by the underlying AVBuffer (and is used to avoid copying the data when creating the AVPackets). Because the allocation of the buffer holding the uncompressed data happens outside of matroska_parse_frame() (if a ContentCompression needs to be reversed), the data is passed as uint8_t ** in order to not leave any dangling pointers behind in matroska_parse_block() should the data need to be freed: In case of errors, said uint8_t ** would be av_freep()'ed in case buf indicated the data to be independently allocated. Yet there is a problem with this: Some codecs (namely WavPack and ProRes) need special handling: Their packets are only stored in Matroska in a stripped form to save space and the demuxer reconstructs full packets. This involved allocating a new, enlarged buffer. And if an error happens when trying to wrap this new buffer into an AVBuffer, this buffer needs to be freed; yet instead the given uint8_t ** (holding the uncompressed, yet still stripped form of the data) would be freed (av_freep()'ed) which certainly leads to a memleak of the new buffer; even worse, in case the track does not use ContentCompression the given uint8_t ** must not be freed as the actual data is owned by an AVBuffer and the data given to matroska_parse_frame() is not the start of the actual allocated buffer at all. Both of these issues are fixed by always freeing the current data in case it is independently allocated. Furthermore, while it would be possible to track whether the pointer from matroska_parse_block() needs to be reset or not, there is no gain in doing so, as the pointer is not used at all afterwards and the sematics are clear: If the data passed to matroska_parse_frame() is independently allocated, then ownership of the data passes to matroska_parse_frame(). So don't pass the data via uint8_t **. Fixes Coverity ID 1462661 (the issue as described by Coverity is btw a false positive: It thinks that this error can be triggered by ProRes with a size of zero after reconstructing the original packets, but the reconstructed packets can't have a size of zero). Signed-off-by: Andreas Rheinhardt --- Sorry for this. Will push this soon if no one objects. I would really appreciate another pair of eyes looking over this. Coverity ID 610554 is btw a false positive stemming from Coverity not knowing that the packet data is always owned by an AVBuffer when ContentCompression is not in use. libavformat/matroskadec.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c index 917c106258..24104837a5 100644 --- a/libavformat/matroskadec.c +++ b/libavformat/matroskadec.c @@ -3414,13 +3414,13 @@ static int matroska_parse_webvtt(MatroskaDemuxContext *matroska, static int matroska_parse_frame(MatroskaDemuxContext *matroska, MatroskaTrack *track, AVStream *st, - AVBufferRef *buf, uint8_t **data, int pkt_size, + AVBufferRef *buf, uint8_t *data, int pkt_size, uint64_t timecode, uint64_t lace_duration, int64_t pos, int is_keyframe, uint8_t *additional, uint64_t additional_id, int additional_size, int64_t discard_padding) { - uint8_t *pkt_data = *data; + uint8_t *pkt_data = data; int res = 0; AVPacket pktl, *pkt = &pktl; @@ -3432,7 +3432,7 @@ static int matroska_parse_frame(MatroskaDemuxContext *matroska, goto fail; } if (!buf) - av_freep(data); + av_free(data); buf = NULL; } @@ -3445,7 +3445,7 @@ static int matroska_parse_frame(MatroskaDemuxContext *matroska, goto fail; } if (!buf) - av_freep(data); + av_free(data); buf = NULL; } @@ -3525,7 +3525,7 @@ FF_ENABLE_DEPRECATION_WARNINGS no_output: fail: if (!buf) - av_freep(data); + av_free(pkt_data); return res; } @@ -3659,7 +3659,7 @@ static int matroska_parse_block(MatroskaDemuxContext *matroska, AVBufferRef *buf if (res) return res; } else { - res = matroska_parse_frame(matroska, track, st, buf, &out_data, + res = matroska_parse_frame(matroska, track, st, buf, out_data, out_size, timecode, lace_duration, pos, !n ? is_keyframe : 0, additional, additional_id, additional_size,