Message ID | 20200917031920.518127-1-andreas.rheinhardt@gmail.com |
---|---|
State | Accepted |
Commit | 93b7f9312d691f4a24a5d48bf98f1e1337cac055 |
Headers | show |
Series | [FFmpeg-devel] avcodec/av1dec: Check tiles sizes, fix assert, don't read bytes bitwise | expand |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
On 9/17/2020 12:19 AM, Andreas Rheinhardt wrote: > Tiles have a size field with a length from one to four bytes. As such it > is not possible to read it all at once with a call to get_bits() as this > only allows to read up to 25 bits; this is guarded by an av_assert2. Yet > this is done by the AV1 decoder in get_tiles_info(). It has been done > despite said size fields being byte-aligned. This commit fixes this by > using the bytestream2 API instead. > > Furthermore, it is now explicitly checked whether the data is > consistent, i.e. whether the data that is supposed to be there extends > beyond the end of the data actually present. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavcodec/av1dec.c | 40 ++++++++++++++++++++-------------------- > 1 file changed, 20 insertions(+), 20 deletions(-) > > diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c > index cb46801202..0bb04a3e44 100644 > --- a/libavcodec/av1dec.c > +++ b/libavcodec/av1dec.c > @@ -21,7 +21,7 @@ > #include "libavutil/pixdesc.h" > #include "avcodec.h" > #include "av1dec.h" > -#include "get_bits.h" > +#include "bytestream.h" > #include "hwconfig.h" > #include "internal.h" > #include "profiles.h" > @@ -205,18 +205,12 @@ static int init_tile_data(AV1DecContext *s) > static int get_tiles_info(AVCodecContext *avctx, const AV1RawTileGroup *tile_group) > { > AV1DecContext *s = avctx->priv_data; > - GetBitContext gb; > + GetByteContext gb; > uint16_t tile_num, tile_row, tile_col; > - uint32_t size = 0, size_bytes = 0, offset = 0; > - int ret; > - > - if ((ret = init_get_bits8(&gb, > - tile_group->tile_data.data, > - tile_group->tile_data.data_size)) < 0) { > - av_log(avctx, AV_LOG_ERROR, "Failed to initialize bitstream reader.\n"); > - return ret; > - } > + uint32_t size = 0, size_bytes = 0; > > + bytestream2_init(&gb, tile_group->tile_data.data, > + tile_group->tile_data.data_size); > s->tg_start = tile_group->tg_start; > s->tg_end = tile_group->tg_end; > > @@ -225,24 +219,28 @@ static int get_tiles_info(AVCodecContext *avctx, const AV1RawTileGroup *tile_gro > tile_col = tile_num % s->raw_frame_header->tile_cols; > > if (tile_num == tile_group->tg_end) { > - s->tile_group_info[tile_num].tile_size = get_bits_left(&gb) / 8; > - s->tile_group_info[tile_num].tile_offset = offset; > + s->tile_group_info[tile_num].tile_size = bytestream2_get_bytes_left(&gb); > + s->tile_group_info[tile_num].tile_offset = bytestream2_tell(&gb); > s->tile_group_info[tile_num].tile_row = tile_row; > s->tile_group_info[tile_num].tile_column = tile_col; > return 0; > } > size_bytes = s->raw_frame_header->tile_size_bytes_minus1 + 1; > - size = get_bits_le(&gb, size_bytes * 8) + 1; > - skip_bits(&gb, size * 8); > - > - offset += size_bytes; > + if (bytestream2_get_bytes_left(&gb) < size_bytes) > + return AVERROR_INVALIDDATA; > + size = 0; > + for (int i = 0; i < size_bytes; i++) > + size |= bytestream2_get_byteu(&gb) << 8 * i; > + if (bytestream2_get_bytes_left(&gb) <= size) > + return AVERROR_INVALIDDATA; > + size++; > > s->tile_group_info[tile_num].tile_size = size; > - s->tile_group_info[tile_num].tile_offset = offset; > + s->tile_group_info[tile_num].tile_offset = bytestream2_tell(&gb); > s->tile_group_info[tile_num].tile_row = tile_row; > s->tile_group_info[tile_num].tile_column = tile_col; > > - offset += size; > + bytestream2_skipu(&gb, size); > } > > return 0; > @@ -778,7 +776,9 @@ static int av1_decode_frame(AVCodecContext *avctx, void *frame, > else > raw_tile_group = &obu->obu.tile_group; > > - get_tiles_info(avctx, raw_tile_group); > + ret = get_tiles_info(avctx, raw_tile_group); > + if (ret < 0) > + goto end; > > if (avctx->hwaccel) { > ret = avctx->hwaccel->decode_slice(avctx, Should be ok.
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of James > Almer > Sent: Thursday, September 17, 2020 11:37 AM > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH] avcodec/av1dec: Check tiles sizes, fix > assert, don't read bytes bitwise > > On 9/17/2020 12:19 AM, Andreas Rheinhardt wrote: > > Tiles have a size field with a length from one to four bytes. As such > > it is not possible to read it all at once with a call to get_bits() as > > this only allows to read up to 25 bits; this is guarded by an > > av_assert2. Yet this is done by the AV1 decoder in get_tiles_info(). > > It has been done despite said size fields being byte-aligned. This > > commit fixes this by using the bytestream2 API instead. > > > > Furthermore, it is now explicitly checked whether the data is > > consistent, i.e. whether the data that is supposed to be there extends > > beyond the end of the data actually present. > > > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > > --- > > libavcodec/av1dec.c | 40 ++++++++++++++++++++-------------------- > > 1 file changed, 20 insertions(+), 20 deletions(-) > > > > diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c index > > cb46801202..0bb04a3e44 100644 > > --- a/libavcodec/av1dec.c > > +++ b/libavcodec/av1dec.c > > @@ -21,7 +21,7 @@ > > #include "libavutil/pixdesc.h" > > #include "avcodec.h" > > #include "av1dec.h" > > -#include "get_bits.h" > > +#include "bytestream.h" > > #include "hwconfig.h" > > #include "internal.h" > > #include "profiles.h" > > @@ -205,18 +205,12 @@ static int init_tile_data(AV1DecContext *s) > > static int get_tiles_info(AVCodecContext *avctx, const AV1RawTileGroup > > *tile_group) { > > AV1DecContext *s = avctx->priv_data; > > - GetBitContext gb; > > + GetByteContext gb; > > uint16_t tile_num, tile_row, tile_col; > > - uint32_t size = 0, size_bytes = 0, offset = 0; > > - int ret; > > - > > - if ((ret = init_get_bits8(&gb, > > - tile_group->tile_data.data, > > - tile_group->tile_data.data_size)) < 0) { > > - av_log(avctx, AV_LOG_ERROR, "Failed to initialize bitstream reader.\n"); > > - return ret; > > - } > > + uint32_t size = 0, size_bytes = 0; > > > > + bytestream2_init(&gb, tile_group->tile_data.data, > > + tile_group->tile_data.data_size); > > s->tg_start = tile_group->tg_start; > > s->tg_end = tile_group->tg_end; > > > > @@ -225,24 +219,28 @@ static int get_tiles_info(AVCodecContext *avctx, > const AV1RawTileGroup *tile_gro > > tile_col = tile_num % s->raw_frame_header->tile_cols; > > > > if (tile_num == tile_group->tg_end) { > > - s->tile_group_info[tile_num].tile_size = get_bits_left(&gb) / 8; > > - s->tile_group_info[tile_num].tile_offset = offset; > > + s->tile_group_info[tile_num].tile_size = > bytestream2_get_bytes_left(&gb); > > + s->tile_group_info[tile_num].tile_offset = > > + bytestream2_tell(&gb); > > s->tile_group_info[tile_num].tile_row = tile_row; > > s->tile_group_info[tile_num].tile_column = tile_col; > > return 0; > > } > > size_bytes = s->raw_frame_header->tile_size_bytes_minus1 + 1; > > - size = get_bits_le(&gb, size_bytes * 8) + 1; > > - skip_bits(&gb, size * 8); > > - > > - offset += size_bytes; > > + if (bytestream2_get_bytes_left(&gb) < size_bytes) > > + return AVERROR_INVALIDDATA; > > + size = 0; > > + for (int i = 0; i < size_bytes; i++) > > + size |= bytestream2_get_byteu(&gb) << 8 * i; > > + if (bytestream2_get_bytes_left(&gb) <= size) > > + return AVERROR_INVALIDDATA; > > + size++; > > > > s->tile_group_info[tile_num].tile_size = size; > > - s->tile_group_info[tile_num].tile_offset = offset; > > + s->tile_group_info[tile_num].tile_offset = > > + bytestream2_tell(&gb); > > s->tile_group_info[tile_num].tile_row = tile_row; > > s->tile_group_info[tile_num].tile_column = tile_col; > > > > - offset += size; > > + bytestream2_skipu(&gb, size); > > } > > > > return 0; > > @@ -778,7 +776,9 @@ static int av1_decode_frame(AVCodecContext *avctx, > void *frame, > > else > > raw_tile_group = &obu->obu.tile_group; > > > > - get_tiles_info(avctx, raw_tile_group); > > + ret = get_tiles_info(avctx, raw_tile_group); > > + if (ret < 0) > > + goto end; > > > > if (avctx->hwaccel) { > > ret = avctx->hwaccel->decode_slice(avctx, > > Should be ok. LGTM. Thanks > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org > with subject "unsubscribe".
diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c index cb46801202..0bb04a3e44 100644 --- a/libavcodec/av1dec.c +++ b/libavcodec/av1dec.c @@ -21,7 +21,7 @@ #include "libavutil/pixdesc.h" #include "avcodec.h" #include "av1dec.h" -#include "get_bits.h" +#include "bytestream.h" #include "hwconfig.h" #include "internal.h" #include "profiles.h" @@ -205,18 +205,12 @@ static int init_tile_data(AV1DecContext *s) static int get_tiles_info(AVCodecContext *avctx, const AV1RawTileGroup *tile_group) { AV1DecContext *s = avctx->priv_data; - GetBitContext gb; + GetByteContext gb; uint16_t tile_num, tile_row, tile_col; - uint32_t size = 0, size_bytes = 0, offset = 0; - int ret; - - if ((ret = init_get_bits8(&gb, - tile_group->tile_data.data, - tile_group->tile_data.data_size)) < 0) { - av_log(avctx, AV_LOG_ERROR, "Failed to initialize bitstream reader.\n"); - return ret; - } + uint32_t size = 0, size_bytes = 0; + bytestream2_init(&gb, tile_group->tile_data.data, + tile_group->tile_data.data_size); s->tg_start = tile_group->tg_start; s->tg_end = tile_group->tg_end; @@ -225,24 +219,28 @@ static int get_tiles_info(AVCodecContext *avctx, const AV1RawTileGroup *tile_gro tile_col = tile_num % s->raw_frame_header->tile_cols; if (tile_num == tile_group->tg_end) { - s->tile_group_info[tile_num].tile_size = get_bits_left(&gb) / 8; - s->tile_group_info[tile_num].tile_offset = offset; + s->tile_group_info[tile_num].tile_size = bytestream2_get_bytes_left(&gb); + s->tile_group_info[tile_num].tile_offset = bytestream2_tell(&gb); s->tile_group_info[tile_num].tile_row = tile_row; s->tile_group_info[tile_num].tile_column = tile_col; return 0; } size_bytes = s->raw_frame_header->tile_size_bytes_minus1 + 1; - size = get_bits_le(&gb, size_bytes * 8) + 1; - skip_bits(&gb, size * 8); - - offset += size_bytes; + if (bytestream2_get_bytes_left(&gb) < size_bytes) + return AVERROR_INVALIDDATA; + size = 0; + for (int i = 0; i < size_bytes; i++) + size |= bytestream2_get_byteu(&gb) << 8 * i; + if (bytestream2_get_bytes_left(&gb) <= size) + return AVERROR_INVALIDDATA; + size++; s->tile_group_info[tile_num].tile_size = size; - s->tile_group_info[tile_num].tile_offset = offset; + s->tile_group_info[tile_num].tile_offset = bytestream2_tell(&gb); s->tile_group_info[tile_num].tile_row = tile_row; s->tile_group_info[tile_num].tile_column = tile_col; - offset += size; + bytestream2_skipu(&gb, size); } return 0; @@ -778,7 +776,9 @@ static int av1_decode_frame(AVCodecContext *avctx, void *frame, else raw_tile_group = &obu->obu.tile_group; - get_tiles_info(avctx, raw_tile_group); + ret = get_tiles_info(avctx, raw_tile_group); + if (ret < 0) + goto end; if (avctx->hwaccel) { ret = avctx->hwaccel->decode_slice(avctx,
Tiles have a size field with a length from one to four bytes. As such it is not possible to read it all at once with a call to get_bits() as this only allows to read up to 25 bits; this is guarded by an av_assert2. Yet this is done by the AV1 decoder in get_tiles_info(). It has been done despite said size fields being byte-aligned. This commit fixes this by using the bytestream2 API instead. Furthermore, it is now explicitly checked whether the data is consistent, i.e. whether the data that is supposed to be there extends beyond the end of the data actually present. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavcodec/av1dec.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-)