From patchwork Thu Oct 17 08:29:42 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 15802 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 33D81449FA2 for ; Thu, 17 Oct 2019 11:30:52 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 1B6EF68A761; Thu, 17 Oct 2019 11:30:52 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 1DD5E68A6D2 for ; Thu, 17 Oct 2019 11:30:47 +0300 (EEST) Received: by mail-wr1-f65.google.com with SMTP id w18so718937wrt.3 for ; Thu, 17 Oct 2019 01:30:47 -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:in-reply-to:references :mime-version:content-transfer-encoding; bh=w0ih5ehayjqLCYFSo4Kw3t3uxmCVk/dq28rNN9IFRgo=; b=qmaVYdvmCfvwooqZQ6Sxf9o1Z/DuBm6qbixLVtScKKtiNqxun6sm7l7vEdfhdd5gJZ /ICR/gqimThlktylyTHIi1AMS1u7wys6BAPYg9nNaxZF7q5IQuwJ5pPaa7A7mb4SW0oM 3IPShIFsygZ4drY+NQUsua12+piMRZ3qgldKiBbWQo0XqgKOvR7ZP3yIA2j2KllQVfoS agIMmthSuXy/qi860T84lp0VTyGnPcvojss4gmPvOSPe/9IElQ+vfYHRQmxl81yGA5cX BEYizim77to3Y6Guge9Vb2qiL0J3rDwYR2ZsWGPbA1aw2Bm0HrznPgwokosO8NXGyjDb n7dA== 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:in-reply-to :references:mime-version:content-transfer-encoding; bh=w0ih5ehayjqLCYFSo4Kw3t3uxmCVk/dq28rNN9IFRgo=; b=goOIjMnDlI7Pqu+GvWGtr6J6H4fWTgs0xxW90NDgdhwWekEv4H+o6qESYYkualCYBI IaCuMH/wfyrI+r5bFvRtV5JHWFq5dOqd98yT1EWpvKjZ+UmXkd934YfcNzGof+DvdnDK 9VCZTsintOYwoxgCYiNlANklb4X6XgJmvbn4KJ8xdz78PRkEy4HDXY+kk0/+GThTFkbc QrYFtm9u3kt0rGwOP0CjyLXQZt8y3me0FxTcr0/eIl+Gfp3Ux9IukEaDaIL8AHu0HIEp 5uJNu7Bs+9JkhKy/YgbzWI+dR43fSb5hOUtHgOtuZ1/cqkb+MmoTINihMPtbRe49jU5p LJWA== X-Gm-Message-State: APjAAAVEGjZd9OrCx5Qd5gyIwAvlD8k5PMMTnscpd8uoGROoTFY3W+IW ZnKxFvZSCRQdtZ4rkpz6M0spGreT X-Google-Smtp-Source: APXvYqzpQE9TysP4a9KqcxDjFhu765+1I/z7ltgD0cXPsDl35VPhpCLxuTJ0YQEhX+lpUn8PVrChqw== X-Received: by 2002:adf:f606:: with SMTP id t6mr1978904wrp.196.1571301046356; Thu, 17 Oct 2019 01:30:46 -0700 (PDT) Received: from sblaptop.fritz.box (ipbcc08937.dynamic.kabel-deutschland.de. [188.192.137.55]) by smtp.gmail.com with ESMTPSA id l7sm1369273wrv.77.2019.10.17.01.30.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 17 Oct 2019 01:30:45 -0700 (PDT) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Thu, 17 Oct 2019 10:29:42 +0200 Message-Id: <20191017082945.13534-12-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20191017082945.13534-1-andreas.rheinhardt@gmail.com> References: <20191017082945.13534-1-andreas.rheinhardt@gmail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 12/15] h264_mp4toannexb: Improve overread checks 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" 1. Left shifts of signed values are undefined as soon as the sign bit is involved. Therefore make nal_size an uint32_t and drop the check for whether it is < 0. 2. The two checks for overreads (whether the length field is contained in the packet and whether the actual unit is contained in the packet) can be combined into one because the packet is padded, i.e. a potential overread caused by reading the length field without checking whether said length field is actually part of the packet's buffer is allowed as one always stays within the padding. But one has to be aware of a pitfall: The comparison must be performed in (at least) int64_t as otherwise buf_end - buf might be promoted to uint32_t in which case an already occured overread would appear as a very large number. A comment explaining this has been added, too. 3. Futhermore, the earlier code returned the wrong error code. This has been fixed, too. Fixes #8290. Signed-off-by: Andreas Rheinhardt --- libavcodec/h264_mp4toannexb_bsf.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/libavcodec/h264_mp4toannexb_bsf.c b/libavcodec/h264_mp4toannexb_bsf.c index 354c9ee4e0..d00c4553c3 100644 --- a/libavcodec/h264_mp4toannexb_bsf.c +++ b/libavcodec/h264_mp4toannexb_bsf.c @@ -171,7 +171,7 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *opkt) AVPacket *in; uint8_t unit_type, new_idr, sps_seen, pps_seen; - int32_t nal_size; + uint32_t nal_size; const uint8_t *buf; const uint8_t *buf_end; uint8_t *out; @@ -202,18 +202,19 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *opkt) out_size = 0; do { - ret= AVERROR(EINVAL); - if (buf + s->length_size > buf_end) - goto fail; - + /* possible overread ok due to padding */ for (nal_size = 0, i = 0; ilength_size; i++) nal_size = (nal_size << 8) | buf[i]; buf += s->length_size; unit_type = *buf & 0x1f; - if (nal_size > buf_end - buf || nal_size < 0) - goto fail; + /* This check requires the cast as the right side might + * otherwise be promoted to an unsigned value. */ + if ((int64_t)nal_size > buf_end - buf) { + ret = AVERROR_INVALIDDATA; + goto fail; + } if (unit_type == H264_NAL_SPS) sps_seen = new_idr = 1;