From patchwork Thu Mar 25 15:49:56 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 26604 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 8169744B722 for ; Thu, 25 Mar 2021 17:50:48 +0200 (EET) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 6B81568AB8F; Thu, 25 Mar 2021 17:50:48 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-ed1-f53.google.com (mail-ed1-f53.google.com [209.85.208.53]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 38F3A68A817 for ; Thu, 25 Mar 2021 17:50:41 +0200 (EET) Received: by mail-ed1-f53.google.com with SMTP id b16so2959480eds.7 for ; Thu, 25 Mar 2021 08:50:41 -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:reply-to :mime-version:content-transfer-encoding; bh=ASJS4Gt91MPaBkMNCtuaSq5AaHGUy90nZtkCg1/yvHg=; b=TD7Uf0zMJ9usuVIFiCRkmcV7V1iNs01liCK+v6oenwXJTsApEfzStqbeBYcRSJxo2T izXSYC8ANlKDYNs2qBhIOd98aj8XGNuAlU8vDzaIbYS/UpzuzWRsqo4lz//6jjZ6tuHQ SM2oWUn8f1eHnVbD5T5o1uwQXa4FtkKF6ID+njgspDHJuStL3BGnpjLntElBQAn/E5MD Xar6DLY9V9Jmk8aTocbnWJIgmjxrxAEpi7v6Fz23RhXeh9Rt8J7G/VuKg3N72i9onUlq n9L2NLfucwCAzU0AmEjRyE23pXAn0kyc0cRJ4NGm5Ie55zmsAENSISv2kFdNlSpyNNfA Y5lA== 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:reply-to:mime-version:content-transfer-encoding; bh=ASJS4Gt91MPaBkMNCtuaSq5AaHGUy90nZtkCg1/yvHg=; b=CrGp2DsaR3pXTOYW3kpc03Xlq3l82QixTcnThJkTqlt+jNd+bCURKONpAON/oxYyBs idzGaOWR1vlGIBV2ER44jMFCHmdQ37XHH4F8JITmOBrCUrK4piVXWihD7ZyemCrbEiqq n+RsB8W0zpKKWqDxbq8UYkeHIkszJYpwZvfGNB9jPwf/S1KFhIxjtjQDDBjqprHCmbdf Tg05DWPhAyklawg80k/8HRjyB9KS7czOKnaK1wmvuVdhjFSmx6DZuJtDJUceB/swf1Gw KO0tH0KhfNtzr8PXFxa7Fh8xVkaH2YeCcn8ZDc+tF1vQsaGUyDJVejWfsrrS3kJKF27D fVsA== X-Gm-Message-State: AOAM533twF7He4/iMEOTTdD4L6ZItcgReZG8rbH01p4HH9+Nrrrck+TM DR8G+BgvITjLeWtQNYGLln7tF03BoZb+Lg== X-Google-Smtp-Source: ABdhPJx6kirDcVsggVrrhdnMxvjYkxhz9JHlIN7pUoxiv24RJ6FlCQTqY7YdvCzTVlKXXh0cUlUN3A== X-Received: by 2002:aa7:c857:: with SMTP id g23mr9909680edt.86.1616687440409; Thu, 25 Mar 2021 08:50:40 -0700 (PDT) Received: from sblaptop.fritz.box (ipbcc08960.dynamic.kabel-deutschland.de. [188.192.137.96]) by smtp.gmail.com with ESMTPSA id o6sm2859103edw.24.2021.03.25.08.50.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 25 Mar 2021 08:50:39 -0700 (PDT) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Thu, 25 Mar 2021 16:49:56 +0100 Message-Id: <20210325154956.2405162-12-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.27.0 In-Reply-To: <20210325154956.2405162-1-andreas.rheinhardt@gmail.com> References: <20210325154956.2405162-1-andreas.rheinhardt@gmail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 12/12] avcodec/put_bits: Don't set size_in_bits, fix overflow 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" A PutBitContext has a field called size_in_bits which is set to the context's bitsize init_put_bits(); but it isn't used at all (the PutBits API uses pointers directly and not bit indexes), so remove it (due to ABI concerns the actual element is only removed at the next bump). Furthermore, the multiplication inherent in setting this field can lead to undefined integer overflows. This is particularly true for FFV1, which uses a very big worst-case buffer (37*4*width*height; even ordinary 1080p triggers an overflow). Ticket #8350 is about this overflow which this commit fixes. This means that the effective range of the PutBits API is no longer restricted by the /8 as long as one isn't using put_bits_(count|left). Signed-off-by: Andreas Rheinhardt --- 1. One can use the full range of int (or of whatever type we choose) and check for whether enough bits are left with a function like this: static inline int put_bits_is_left(const PutBitContext *s, int n) { s->buf_end - s->buf_ptr >= (BUF_BITS - s->bit_left + n + 7) / 8; } (This presumes that n is not nearly the maximum of its type.) 2. I see three more ways in which the PutBits API can be improved: a) Don't use av_log/asserts in case of overreads; instead handle it via a context field for overread similar to the bytestream API. b) Integrate the PutBits API and the bytestream API: When one knows that the current position in the bitstream is byte-aligned and the PutBits context is flushed, one should be able to use the PutBits API with an API similar to the one for PutByteContexts. ff_mjpeg_encode_picture_header() is an example where this would be beneficial. c) If the output buffer is suitably padded, one could allow slight overwrites. In this case the check s->buf_end - s->buf_ptr >= sizeof(BitBuf) could be replaced by s->buf_ptr < s->buf_end (one could also add a new pointer to the context for the effective end (s->buf_end - FFMIN(buffer_size, sizeof(BitBuf))) and compare with that instead, but this seems to be a bit too involved). What do others think about this? libavcodec/put_bits.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/libavcodec/put_bits.h b/libavcodec/put_bits.h index e8bc86a82c..15c2650724 100644 --- a/libavcodec/put_bits.h +++ b/libavcodec/put_bits.h @@ -52,7 +52,9 @@ typedef struct PutBitContext { BitBuf bit_buf; int bit_left; uint8_t *buf, *buf_ptr, *buf_end; +#if LIBAVCODEC_VERSION_MAJOR < 59 int size_in_bits; +#endif } PutBitContext; /** @@ -69,7 +71,6 @@ static inline void init_put_bits(PutBitContext *s, uint8_t *buffer, buffer = NULL; } - s->size_in_bits = 8 * buffer_size; s->buf = buffer; s->buf_end = s->buf + buffer_size; s->buf_ptr = s->buf; @@ -120,7 +121,6 @@ static inline void rebase_put_bits(PutBitContext *s, uint8_t *buffer, s->buf_end = buffer + buffer_size; s->buf_ptr = buffer + (s->buf_ptr - s->buf); s->buf = buffer; - s->size_in_bits = 8 * buffer_size; } /** @@ -414,7 +414,6 @@ static inline void set_put_bits_buffer_size(PutBitContext *s, int size) { av_assert0(size <= INT_MAX/8 - BUF_BITS); s->buf_end = s->buf + size; - s->size_in_bits = 8*size; } /**