From patchwork Mon Jul 22 03:27:14 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 14026 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 29C79449A9E for ; Mon, 22 Jul 2019 06:34:05 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 0CA6F68A380; Mon, 22 Jul 2019 06:34:05 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wm1-f68.google.com (mail-wm1-f68.google.com [209.85.128.68]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 747E468A380 for ; Mon, 22 Jul 2019 06:33:58 +0300 (EEST) Received: by mail-wm1-f68.google.com with SMTP id s3so33642697wms.2 for ; Sun, 21 Jul 2019 20:33:58 -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=ymnupz1TyEstzIUC2/Noint9edwApzH5JOdIU/NxhtQ=; b=bG9t7EwvVWjc6Po2vT2eUR1OmRfbWzKIbuTZclfIZCZtGaKA71P6gJXuiZcM/OGTW9 0tVBhuD2nny7N/Q5FFH7CzmypUP/Zwo4KkyVEEEyja+M+MlwlR37NDF5btosNUWbkdFo knhruNN9o//oaMlSzZHJV5hTu+zJ8K4tzKiE7O8tktv9RKDuq7hfe5BGPYZJyDIrQCgl YbUABMzn+3FOo6i1bvkZnXmL58SIpVAYOLrzoOwES+cOUsChDgOd++x4FVp4ZaKsGR2E Bli7cuM5IkX2L92yn28AIccusqllva5qxIxsd7nbTLuSJM/Mxu8K336mvruwchzUO+P2 1uMQ== 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=ymnupz1TyEstzIUC2/Noint9edwApzH5JOdIU/NxhtQ=; b=C+auyhCuIx/LWJohVR8iDdldzdpRZe1GeBoIbxIlOCbu0VqZzicEzOC6/F7BLCHCLk SfX9XOBtV4rG5CqzZuVMeX43FbJm3Hea+5STnw78SP+gLqPooqONP/ga21gOCFfyszy/ mF4Nge/zDDZffKNf2zpmmJHb3DWXk9c8elld3f5VjF+CNW7/PYUhkqQZwo3XqIXO5sfS cKdSWxVXmLnJOzdWze7xt/SFaE98pMQTBAodqtOg/bEAA9gTq8Ih73LsCzMmQH9FJxKq OiE1J/F7GEpdbBVbJ9+h2QJq73gPh8zU3KYeWcGvtu9z+WTQKHmGp1eHNWQYAdL1iqjK KQ1g== X-Gm-Message-State: APjAAAVJZslicNC3Q95TKekvK1z3GWsHh63X0FAb5d0zHx6W8Fu6sO05 Fo+RVwpaAASHSaWjvTO1Iv/MHJrC X-Google-Smtp-Source: APXvYqxksGoBEcJ9wtjg7SmRUqpRR7fmdeRZrHV9siLK7ngTsnpjk8+nlKiCoeY5h/HF1SOoT1dOGw== X-Received: by 2002:a7b:c310:: with SMTP id k16mr22515795wmj.133.1563766437777; Sun, 21 Jul 2019 20:33:57 -0700 (PDT) Received: from localhost.localdomain (ipbcc08b8f.dynamic.kabel-deutschland.de. [188.192.139.143]) by smtp.gmail.com with ESMTPSA id t1sm49233202wra.74.2019.07.21.20.33.57 (version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256); Sun, 21 Jul 2019 20:33:57 -0700 (PDT) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Mon, 22 Jul 2019 05:27:14 +0200 Message-Id: <20190722032716.40032-3-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.21.0 In-Reply-To: <20190722032716.40032-1-andreas.rheinhardt@gmail.com> References: <20190722032716.40032-1-andreas.rheinhardt@gmail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 3/4] h264_mp4toannexb: Improve overread checks II 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 negative 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. Signed-off-by: Andreas Rheinhardt --- libavcodec/h264_mp4toannexb_bsf.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/libavcodec/h264_mp4toannexb_bsf.c b/libavcodec/h264_mp4toannexb_bsf.c index 076dfa31a7..ef0ccacf29 100644 --- a/libavcodec/h264_mp4toannexb_bsf.c +++ b/libavcodec/h264_mp4toannexb_bsf.c @@ -170,8 +170,7 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out) AVPacket *in; uint8_t unit_type; - int32_t nal_size; - uint32_t cumul_size = 0; + uint32_t cumul_size = 0, nal_size; const uint8_t *buf; const uint8_t *buf_end; int buf_size; @@ -193,18 +192,19 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out) buf_end = in->data + in->size; 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) + /* 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(EINVAL); goto fail; + } if (unit_type == H264_NAL_SPS) s->idr_sps_seen = s->new_idr = 1;