From patchwork Wed Jul 24 17:15:56 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 14060 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 8818E448079 for ; Wed, 24 Jul 2019 20:24:55 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 6706868AA22; Wed, 24 Jul 2019 20:24:55 +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 2581E6898AC for ; Wed, 24 Jul 2019 20:24:49 +0300 (EEST) Received: by mail-wm1-f68.google.com with SMTP id g67so38237189wme.1 for ; Wed, 24 Jul 2019 10:24:49 -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=1+6i018rb4TwWIMs5c5Q396GqrkrZrrjcarKFJ3tOE4=; b=QCo6R4hmfN+W4rSadwmx6ptJmnC3/SJXQ31D6LpDn3lzB59M/E8vI4DfLM9MZXpInP Wz227OG9iAtblKftvZWPsu0C5Upq4F8Owlyu9NuZ1cm2IkxfU+xihP3pOFlkfhAIjlw2 b/oVc0mVqJKnC85fcAeeKOqnuMG848/urfPoLnb5hOqKMB9ZK+oPYZVgKS3I6mBGZ31i LeJtPhlpwmPdxDUyudYMpXlloba1XGq4cI/wl5odZGICgdS6DW67ZkVo4mYcaTq/US4i vuVKEvqNGMOkXAOhuOUU7zMzZ/jaqMUrPZ8zkePryKlQhI6w0n/lcwn2S4fire3VWrT7 hnaQ== 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=1+6i018rb4TwWIMs5c5Q396GqrkrZrrjcarKFJ3tOE4=; b=oi9goHRT1sLQBaaeDtTaAZdPUOksze9uf9T1ddJ+z8ctVvUK/SN+2MojVezhLiRmSJ 3mVpnhVqySj3IezIRZpmjvGU8g1PBC459eRytui8itxtrBUeuoCQV2ljYUEDOdEEPFrZ 4sugSKzq4YvMlGjsxkI1A5dwl9WqBw9tP/6hgg0HQOxIPwHx7SImlUPd4ChTg/FgHN93 awa3OD+VOkrHMhuzC2s6Jr73FKSRUX5KH1ELFqBvSOJKqhCvBv4so/qEruj6SpYHqdvR gg29UWwgem5KFKMvyczfxhRbZslkuwPiphNufl0NhhDT09NvC4/0I5+iFEkAavgCZHRQ acMQ== X-Gm-Message-State: APjAAAU0hn5VjgV+7YDWkWpYAin/4Ov/pQxz7l+hf/vxGB5BuzpwIjFD uCylK+gKYIDh7LnMqtPTrmCEqz5p X-Google-Smtp-Source: APXvYqxMrSMEHudVzqlE2+Qkdo3KmlkfUcD2rDhY+mR4MdC/B/HVGTrIxpYA6vOCSQyOcjU854G2PA== X-Received: by 2002:a1c:200a:: with SMTP id g10mr70296772wmg.160.1563989087873; Wed, 24 Jul 2019 10:24:47 -0700 (PDT) Received: from localhost.localdomain (ipbcc08b8f.dynamic.kabel-deutschland.de. [188.192.139.143]) by smtp.gmail.com with ESMTPSA id l8sm82829930wrg.40.2019.07.24.10.24.47 (version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256); Wed, 24 Jul 2019 10:24:47 -0700 (PDT) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Wed, 24 Jul 2019 19:15:56 +0200 Message-Id: <20190724171557.10037-4-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.21.0 In-Reply-To: <20190724171557.10037-1-andreas.rheinhardt@gmail.com> References: <20190724074358.GU3219@michaelspb> <20190724171557.10037-1-andreas.rheinhardt@gmail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 5/6] 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 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 0f46ad907c..5d9b71000e 100644 --- a/libavcodec/h264_mp4toannexb_bsf.c +++ b/libavcodec/h264_mp4toannexb_bsf.c @@ -172,8 +172,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; @@ -195,18 +194,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;