From patchwork Mon Nov 18 07:47:58 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 16312 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 583C4447CA6 for ; Mon, 18 Nov 2019 09:48:18 +0200 (EET) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 3538D68A633; Mon, 18 Nov 2019 09:48:18 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 4912B689970 for ; Mon, 18 Nov 2019 09:48:11 +0200 (EET) Received: by mail-wm1-f65.google.com with SMTP id b11so16224882wmb.5 for ; Sun, 17 Nov 2019 23:48:11 -0800 (PST) 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=fv2tvfM209QMwFmquAobpba6aam1MYmHxpmutt3t0Bw=; b=HytDWRSEd1BD1/xEQKTgaVUCztJEwOVJ81qNJ8zTg83/iuIuHmOTa8zX1Kpkc2Gmo3 8YsTAaTWXi0krCjRgZMCy5CJpHxHf5pPVzT4v5msrpPNa2c7pmCgjJv2BfR6dBXVerzG m3Q3dB1wts1PQm13oaeBBvjqGgayNqum9X/AOUBz5VhIJzWvn0YapXTVzxN1TAOMbJ8P mN2vPpq/Hm9xRMzayin34yBJCrkWbeG7PFxKUtld8lXQEu8xoA0b8AFoY+Raoyjo6PPu N/RPXf3NOkcHFmUYnYixSwDvknutsLkFL9WEdccYQtqdlkNdIXCg0PiNU6JOpZrLynzA e7Hw== 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=fv2tvfM209QMwFmquAobpba6aam1MYmHxpmutt3t0Bw=; b=hh+yFhrlqg5xypJQDqpIYOflNf2Ll6O62ZqAgwAMLvc8oR6PMVBGJzvQAlEF7FrKiP 4JMqKFhIRnsA2nzsXUupGkbHtVcVyjfLgPr3NRCYIFlgN5rdW6+HJHp5dn14wrbzUpOP Glw/EPQ2GpUcrdfQ+C9NKek+rkODNvcgm4v3O1s6H/7F44iC0M2eHTkoDLgvQaIMtuqs 0E+yeJSS+ZGbRlv+nzTLCupA3/D9u5+6EwefHBnNJ91sPKjKD/6eDQVCMefg6NZIj753 pok9BgkOBNKNFLk0umkzDgG/8tJ7f+ZdLGruogi0LtS45SaojEzDZ32LcVT8OG6/pQ5e ZKRQ== X-Gm-Message-State: APjAAAXLx5Cz6DiVoyNoECoYt6F1DNypz7ThC2n5vU84bDKp0Lde6Gfs zTS8bO9WlpL7WUjjcdTw/XJ0G9Zc X-Google-Smtp-Source: APXvYqwV3FCGtCVSXmaCZDLtoNMT6pcOWstqabD7kQYnbXxpI6ICguQWDR/Nwmv2tBX1MpLjT4nBtw== X-Received: by 2002:a05:600c:2143:: with SMTP id v3mr9639291wml.3.1574063290557; Sun, 17 Nov 2019 23:48:10 -0800 (PST) Received: from sblaptop.fritz.box (ipbcc08e23.dynamic.kabel-deutschland.de. [188.192.142.35]) by smtp.gmail.com with ESMTPSA id 200sm21842648wme.32.2019.11.17.23.48.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 17 Nov 2019 23:48:09 -0800 (PST) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Mon, 18 Nov 2019 08:47:58 +0100 Message-Id: <20191118074759.6562-1-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.20.1 MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 1/2] avcodec/cbs: Fix potential double-free when adding unit fails 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" ff_cbs_insert_unit_data() has two modes of operation: It can insert a unit with a newly created reference to an already existing AVBuffer; or it can take a buffer and create an AVBuffer for it. Said buffer will then become owned by the unit lateron. A potential memleak/double-free exists in the second case, because if creating the AVBuffer fails, the function immediately returns, but when it fails lateron, the supplied buffer will be freed. The caller has no way to distinguish between these two outcomes. The only such caller (cbs_jpeg_split_fragment() in cbs_jpeg.c) opted for a potential double-free. This commit changes this by explicitly stating that a non-refcounted buffer will be freed on error. The aforementioned caller has been brought in line with this. Fixes CID 1452623. Signed-off-by: Andreas Rheinhardt --- Actually CID 1452623 is a false positive: Coverity thinks that the frsgment's data buffer is NULL, which it never is (or we wouldn't be here). libavcodec/cbs.c | 5 ++++- libavcodec/cbs.h | 3 ++- libavcodec/cbs_jpeg.c | 5 +---- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c index 0badb192d9..0bd5e1ac5d 100644 --- a/libavcodec/cbs.c +++ b/libavcodec/cbs.c @@ -775,8 +775,11 @@ int ff_cbs_insert_unit_data(CodedBitstreamContext *ctx, data_ref = av_buffer_ref(data_buf); else data_ref = av_buffer_create(data, data_size, NULL, NULL, 0); - if (!data_ref) + if (!data_ref) { + if (!data_buf) + av_free(data); return AVERROR(ENOMEM); + } err = cbs_insert_unit(ctx, frag, position); if (err < 0) { diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h index cdb777d111..9ca1fbd609 100644 --- a/libavcodec/cbs.h +++ b/libavcodec/cbs.h @@ -376,7 +376,8 @@ int ff_cbs_insert_unit_content(CodedBitstreamContext *ctx, * Insert a new unit into a fragment with the given data bitstream. * * If data_buf is not supplied then data must have been allocated with - * av_malloc() and will become owned by the unit after this call. + * av_malloc() and will on success become owned by the unit after this + * call or freed on error. */ int ff_cbs_insert_unit_data(CodedBitstreamContext *ctx, CodedBitstreamFragment *frag, diff --git a/libavcodec/cbs_jpeg.c b/libavcodec/cbs_jpeg.c index b189cbd9b7..b52e5c5823 100644 --- a/libavcodec/cbs_jpeg.c +++ b/libavcodec/cbs_jpeg.c @@ -225,11 +225,8 @@ static int cbs_jpeg_split_fragment(CodedBitstreamContext *ctx, err = ff_cbs_insert_unit_data(ctx, frag, unit, marker, data, data_size, data_ref); - if (err < 0) { - if (!data_ref) - av_freep(&data); + if (err < 0) return err; - } if (next_marker == -1) break;