From patchwork Fri Jul 31 10:39:45 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 21400 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 EF5FE44A5EB for ; Fri, 31 Jul 2020 13:40:05 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id CA42C68B7FA; Fri, 31 Jul 2020 13:40:05 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-ed1-f65.google.com (mail-ed1-f65.google.com [209.85.208.65]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 1BBFE68B7AD for ; Fri, 31 Jul 2020 13:39:59 +0300 (EEST) Received: by mail-ed1-f65.google.com with SMTP id c15so12372380edj.3 for ; Fri, 31 Jul 2020 03:39:59 -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:mime-version :content-transfer-encoding; bh=v5KUVzANRNzTbiQ8hsvLh8ic3NpCulEjBy353qp0FE8=; b=SVnjuurCUJWQgCIeN3NgLvB1xJtfeOqtiWKb20L9MaXqoyWoy0QyKPHoi8/tSctoGv lFCFTx+wLD1I3lSauRMDFRyzEn44ogCJ7FHtwffxJXqNFeb4KGbkc0+nnzNCAk37r1sv E0sy60Qk0RiayvOwOu+eP8W904dXsJefvDi/ONU0ng+v3HkUp2L1ReeASQQAaWQmcccC dmLtpemoFVCvhrO64aOljuwwEvHwNHITsyjSXpuMHt4/PWmcyJu/4jSeb69X8OGYCYDo 7kmlPCj58GRtPwLmyEk+J3niopKw3f7N6NDir7bouPe/2bk666NE8HoSs9syWCa0PtPt /sJQ== 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=v5KUVzANRNzTbiQ8hsvLh8ic3NpCulEjBy353qp0FE8=; b=a7L5gr55PuJRpHK5yTKbn17V7ljTBSkTbDkTruq3au2Ktit1RMabEdQwgsQ9soXm32 HL9+zL3vwFzoEd3gWClc0z9/37JXE7y0wKEvRbeL16Dx7Aw/uh+cCxFQL915+sEPodhF GzW1edDYUMcIPTXAKIB+iXg7ZPQkpjQilj9J144nhGjElEVG0D7z3IjMBLSrd+dycQ3m h11s7ieUdj6/ph7esLFLscao0kb06676zbrQUD7Z7u1rMUxWX1p5GkQfNk5AAR49Te/v p4aFmUOIxJF7C3nlMH/gK/JD99kc9CX017SCsl3w6L4We6Z7FNc4q4fg4kq3vV9snvSN hqNA== X-Gm-Message-State: AOAM533jGeWSRlZMk5ZONMCuL3vPrnAGScwQRnkGQGzxsRdRR5Qbmqrd JHpz6Emx1qJvLQ9RrEhvDXJXS4dt X-Google-Smtp-Source: ABdhPJxtJ8HquJLuo1O9ujvdvzuemwEAFubc8S4O78V8wRMo7/Ekqw6Lfak1VKHs4al3IKHmFRU7Dw== X-Received: by 2002:aa7:db10:: with SMTP id t16mr3082106eds.196.1596191998200; Fri, 31 Jul 2020 03:39:58 -0700 (PDT) Received: from sblaptop.fritz.box (ipbcc10296.dynamic.kabel-deutschland.de. [188.193.2.150]) by smtp.gmail.com with ESMTPSA id k18sm8877891ejp.81.2020.07.31.03.39.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 31 Jul 2020 03:39:57 -0700 (PDT) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Fri, 31 Jul 2020 12:39:45 +0200 Message-Id: <20200731103946.2843-1-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.20.1 MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 1/2] avcodec/put_bits: Make skip_put_bits() less dangerous 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" Before c63c303a1f2b58677d480505ec93a90f77dd25b5 (the commit which introduced a typedef for the type of the buffer of a PutBitContext) skip_put_bits() was as follows: static inline void skip_put_bits(PutBitContext *s, int n) { s->bit_left -= n; s->buf_ptr -= 4 * (s->bit_left >> 5); s->bit_left &= 31; } If s->bit_left was negative after the first subtraction, then the next line will divide this by 32 with rounding towards -inf and multiply by four; the result will be negative, of course. The aforementioned commit changed this to: static inline void skip_put_bits(PutBitContext *s, int n) { s->bit_left -= n; s->buf_ptr -= sizeof(BitBuf) * ((unsigned)s->bit_left / BUF_BITS); s->bit_left &= (BUF_BITS - 1); } Casting s->bit_left to unsigned meant that the rounding is still towards -inf; yet the right side is now always positive (it transformed the arithmetic shift into a logical shift), so that s->buf_ptr will always be decremented (by about UINT_MAX / 8 unless n is huge) which leads to segfaults on further usage and is already undefined pointer arithmetic before that. This can be reproduced with the mpeg4 encoder with the AV_CODEC_FLAG2_NO_OUTPUT flag set. Furthermore, the earlier version as well as the new version share another bug: s->bit_left will be in the range of 0..(BUF_BITS - 1) afterwards, although the assumption throughout the other PutBitContext functions is that it is in the range of 1..BUF_BITS. This might lead to a shift by BUF_BITS in little-endian mode. This has been fixed, too. The new version is furthermore able to skip zero bits, too. Signed-off-by: Andreas Rheinhardt --- 1. skip_put_bits() is still bad even after this: If one skips so few that buf_ptr will not change, the non-skipped bits in the buffer will be wrong in case of a big-endian reader (they would need to be shifted by the number of bits to be skipped for this to work*). If one skips so much that buf_ptr does change, all the valid bits in the buffer will not be output at the place where they should have been output. 2. Since c63c303a1f2b58677d480505ec93a90f77dd25b5 sizeof(BitBuf) is used in several comparisons like s->buf_end - s->buf_ptr >= sizeof(BitBuf) where an immediate (of type int) has been used before. This is a problem if one is already past the end of the buffer, because the left side will (typically) be converted to size_t. With the current API, this can only happen when skip_put_bits() and set_put_bits_buffer_size() are used (or if one modifies the PutBitContext manually). But it would be a problem if we would add an unchecked version of this API for users that do their own checks and if said user would want to use padding in a controlled manner to be able to minimize the amount of checks. 3. Should the "Internal error, put_bits buffer too small" error message without proper logcontext be removed just like the one in get_ue_golomb() was in 39c4b788297b7883d833d68fad3707ce50e01472? (It could of course be used for the av_assert2 message.) *: For a big-endian writer, the already buffered bits occupy the least significant bits of the buffer. If they were already put into their final position, one would need to disallow/special-case writing zero bits with put_bits() (but it could then automatically be used to write up to BIT_BUF bits). libavcodec/put_bits.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libavcodec/put_bits.h b/libavcodec/put_bits.h index ddd97906b2..3ba9549948 100644 --- a/libavcodec/put_bits.h +++ b/libavcodec/put_bits.h @@ -364,13 +364,13 @@ static inline void skip_put_bytes(PutBitContext *s, int n) /** * Skip the given number of bits. * Must only be used if the actual values in the bitstream do not matter. - * If n is 0 the behavior is undefined. + * If n is < 0 the behavior is undefined. */ static inline void skip_put_bits(PutBitContext *s, int n) { - s->bit_left -= n; - s->buf_ptr -= sizeof(BitBuf) * ((unsigned)s->bit_left / BUF_BITS); - s->bit_left &= (BUF_BITS - 1); + unsigned bits = BUF_BITS - s->bit_left + n; + s->buf_ptr += sizeof(BitBuf) * (bits / BUF_BITS); + s->bit_left = BUF_BITS - (bits & (BUF_BITS - 1)); } /**