On Mon, 09. Dec 23:25, Andreas Rheinhardt wrote: > While CBS itself uses size_t for sizes, it relies on other APIs that use > int for their sizes; in particular, AVBuffer uses int for their size > parameters and so does GetBitContext with their number of bits. While > CBS aims to be a safe API, the checks it employed were not sufficient to > prevent overflows: E.g. if the size of a unit was > UINT_MAX / 8, 8 * > said size may be truncated to a positive integer before being passed to > init_get_bits() in which case its return value would not indicate an > error. > > These checks have been improved to really capture these kinds of errors; > furthermore, although the sizes are still size_t, they are now de-facto > restricted to 0..INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinha...@gmail.com> > --- > The check in cbs_insert_unit() can currently not be triggered, because > av_malloc_array makes sure that it doesn't allocate more than INT_MAX; > so the allocation will fail long before we get even close to INT_MAX. > > av1 and H.26x have not been changed, because their split functions > already check the size (in case of H.264 and H.265 this happens in > ff_h2645_packet_split()). > > It would btw be possible to open the GetBitContext generically. The > read_unit function would then get the already opened GetBitContext > as a parameter. > > libavcodec/cbs.c | 6 ++++++ > libavcodec/cbs_jpeg.c | 2 +- > libavcodec/cbs_mpeg2.c | 2 +- > libavcodec/cbs_vp9.c | 2 +- > 4 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c > index 0badb192d9..805049404b 100644 > --- a/libavcodec/cbs.c > +++ b/libavcodec/cbs.c > @@ -206,6 +206,9 @@ static int cbs_fill_fragment_data(CodedBitstreamContext > *ctx, > { > av_assert0(!frag->data && !frag->data_ref); > > + if (size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE) > + return AVERROR(ERANGE); > + > frag->data_ref = > av_buffer_alloc(size + AV_INPUT_BUFFER_PADDING_SIZE); > if (!frag->data_ref) > @@ -693,6 +696,9 @@ static int cbs_insert_unit(CodedBitstreamContext *ctx, > memmove(units + position + 1, units + position, > (frag->nb_units - position) * sizeof(*units)); > } else {
> + if (frag->nb_units == INT_MAX) > + return AVERROR(ERANGE); > + Why did you decide to include this? As you say in your comments it cannot be triggered. I looked over Patch 1-5 and they look good to me. Also, thanks for answering some clarifying questions I had on IRC. -- Andriy _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".