diff mbox series

[FFmpeg-devel,12/12] avcodec/put_bits: Don't set size_in_bits, fix overflow

Message ID 20210325154956.2405162-12-andreas.rheinhardt@gmail.com
State Accepted
Commit e7cbbd90267de2a0ad1b5fa8ccb29ab7bf8a26b8
Headers show
Series [FFmpeg-devel,01/12] avcodec/vorbisenc: Remove always-false check
Related show


Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt March 25, 2021, 3:49 p.m. UTC
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 <andreas.rheinhardt@gmail.com>
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
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 mbox series


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;
     int size_in_bits;
 } 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;