Skip to content

Make image size limit configurable, expose to avifdec #527

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Make image size limit configurable, expose to avifdec #527

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8a6506d
Make image size limit configurable, expose to avifdec
Feb 26, 2021
21167c9
Rename AVIF_MAX_IMAGE_SIZE to AVIF_DEFAULT_MAX_IMAGE_SIZE, set defaul…
Mar 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions apps/avifdec.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ static void syntax(void)
printf(" -u,--upsampling U : Chroma upsampling (for 420/422). automatic (default), fastest, best, nearest, or bilinear\n");
printf(" -i,--info : Decode all frames and display all image information instead of saving to disk\n");
printf(" --ignore-icc : If the input file contains an embedded ICC profile, ignore it (no-op if absent)\n");
printf(" --size-limit C : Specifies the image size limit (in total pixels) that the AV1 codec should tolerate.\n");
printf(" Default: %u, set to 0 to disable. Supported codecs: dav1d.\n", AVIF_MAX_IMAGE_SIZE);
printf("\n");
avifPrintVersions();
}
Expand Down Expand Up @@ -91,6 +93,7 @@ int main(int argc, char * argv[])
avifBool infoOnly = AVIF_FALSE;
avifChromaUpsampling chromaUpsampling = AVIF_CHROMA_UPSAMPLING_AUTOMATIC;
avifBool ignoreICC = AVIF_FALSE;
uint32_t imageSizeLimit = AVIF_MAX_IMAGE_SIZE;

if (argc < 2) {
syntax();
Expand Down Expand Up @@ -161,6 +164,9 @@ int main(int argc, char * argv[])
infoOnly = AVIF_TRUE;
} else if (!strcmp(arg, "--ignore-icc")) {
ignoreICC = AVIF_TRUE;
} else if (!strcmp(arg, "--size-limit")) {
NEXTARG();
imageSizeLimit = strtoul(arg, NULL, 10);
} else {
// Positional argument
if (!inputFilename) {
Expand Down Expand Up @@ -205,6 +211,7 @@ int main(int argc, char * argv[])
avifDecoder * decoder = avifDecoderCreate();
decoder->maxThreads = jobs;
decoder->codecChoice = codecChoice;
decoder->imageSizeLimit = imageSizeLimit;
avifResult decodeResult = avifDecoderReadFile(decoder, avif, inputFilename);
if (decodeResult == AVIF_RESULT_OK) {
printf("Image decoded: %s\n", inputFilename);
Expand Down
9 changes: 9 additions & 0 deletions include/avif/avif.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@ typedef int avifBool;
#define AVIF_SPEED_SLOWEST 0
#define AVIF_SPEED_FASTEST 10

// A maximum image size to avoid out-of-memory errors or integer overflow in
Copy link
Collaborator

@wantehchang wantehchang Feb 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think we should call this "The default maximum image size ..." now

// (32-bit) int or unsigned int arithmetic operations.
#define AVIF_MAX_IMAGE_SIZE (16384 * 16384)
Copy link
Collaborator

@wantehchang wantehchang Feb 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your suggestion of adding DEFAULT to the macro's name is good.


enum avifPlanesFlags
{
AVIF_PLANES_YUV = (1 << 0),
Expand Down Expand Up @@ -701,6 +705,11 @@ typedef struct avifDecoder
avifBool ignoreExif;
avifBool ignoreXMP;

// This represents the maximum size of a image (in pixel count) that the underlying AV1 decoder
Copy link
Collaborator

@wantehchang wantehchang Feb 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: a image => an image

// should attempt to decode. It defaults to AVIF_MAX_IMAGE_SIZE, and can be set to 0 to disable
// the limit. Currently supported codecs: dav1d.
Copy link
Collaborator

@wantehchang wantehchang Feb 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. It would be good to note that other AV1 codecs may have a non-configurable frame size limit. That's the case for libgav1 and libaom.

For libgav1, the frame size limit is hardcoded in the source code, currently INT32_MAX:

  if (frame_header_.upscaled_width > INT32_MAX / frame_header_.height) {
    LIBGAV1_DLOG(ERROR, "Frame dimensions too big: width=%d height=%d.",
                 frame_header_.width, frame_header_.height);
    return false;
  }

For libaom, the frame size limit is a build opton, consisting of three cmake flags. Here is how libaom's source code uses the three corresponding C-preprocessor macros:

static AOM_INLINE void resize_context_buffers(AV1_COMMON *cm, int width,
                                              int height) {
#if CONFIG_SIZE_LIMIT
  if (width > DECODE_WIDTH_LIMIT || height > DECODE_HEIGHT_LIMIT)
    aom_internal_error(&cm->error, AOM_CODEC_CORRUPT_FRAME,
                       "Dimensions of %dx%d beyond allowed size of %dx%d.",
                       width, height, DECODE_WIDTH_LIMIT, DECODE_HEIGHT_LIMIT);
#endif

Note that the clients of libaom cannot change the size limit at run time.

  1. Clarify whether imageSizeLimit applies to grid image size.

uint32_t imageSizeLimit;
Copy link
Collaborator

@wantehchang wantehchang Feb 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. In src/avif.c, avifImageSetDefaults() needs to initialize imageSizeLimit to the default (AVIF_MAX_IMAGE_SIZE).

  2. The use of AVIF_MAX_IMAGE_SIZE in src/read.c needs to be updated. It may need to replaced with imageSizeLimit, depending on whether you also want imageSizeLimit to apply to grid image size. (The current comment suggests imageSizeLimit does not apply to grid image size.)

We also need to make sure all of our multiplications involving width or height do not overflow the integer type used in the arithmetic. I remember I reviewed that before, but it is best to check it again.

Copy link
Contributor Author

@joedrago joedrago Mar 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imageSizeLimit is not a member of avifImage, so bullets 1 and 2 don't apply here, right?

Copy link
Collaborator

@wantehchang wantehchang Mar 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about my confusion. I checked the wrong function. Bullet 1 should read:

  1. In src/read.c, avifDecoderCreate() needs to initialize imageSizeLimit to the default (AVIF_MAX_IMAGE_SIZE).

Bullet 2 is correct. I was referring to the following code in avifParseImageGridBox():

    if ((grid->outputWidth == 0) || (grid->outputHeight == 0) || (grid->outputWidth > (AVIF_MAX_IMAGE_SIZE / grid->outputHeight))) {
        return AVIF_FALSE;
    }

Copy link
Contributor Author

@joedrago joedrago Mar 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. That is a good question.

Do we want imageSizeLimit to be something solely enforced by the underlying AV1 decoders, or do we want to explicitly check these limits even at the container level? For example, if the ispe box returned something that exceeds imageSizeLimit, do we bail out immediately? Do we want a new error of AVIF_RESULT_IMAGE_TOO_LARGE?

Copy link
Collaborator

@wantehchang wantehchang Mar 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imageSizeLimit serves two purposes:

  1. To help prevent integer arithmetic overflow in libavif clients' code. For this purpose, we want to explicitly check the image size limits even at the container level. (I believe we are not doing that now.) I think we should add a new error of AVIF_RESULT_IMAGE_TOO_LARGE.
  2. oss-fuzz tests fail if we allocate too much memory from the heap. (oss-fuzz doesn't care whether we handle out-of-memory errors properly.) For this purpose, we don't need to check the image size limits at the container level, but we do need to check the grid image sizes (because we will call avifImageAllocatePlanes() in avifDecoderDataFillImageGrid()), and we also need the underlying AV1 decoders to enforce the same size limits.


// stats from the most recent read, possibly 0s if reading an image sequence
avifIOStats ioStats;

Expand Down
4 changes: 0 additions & 4 deletions include/avif/internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,10 +330,6 @@ typedef struct avifSequenceHeader
} avifSequenceHeader;
avifBool avifSequenceHeaderParse(avifSequenceHeader * header, const avifROData * sample);

// A maximum image size to avoid out-of-memory errors or integer overflow in
// (32-bit) int or unsigned int arithmetic operations.
#define AVIF_MAX_IMAGE_SIZE (16384 * 16384)

#ifdef __cplusplus
} // extern "C"
#endif
Expand Down
4 changes: 1 addition & 3 deletions src/codec_dav1d.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ static avifBool dav1dCodecOpen(avifCodec * codec, avifDecoder * decoder)
// Give all available threads to decode a single frame as fast as possible
codec->internal->dav1dSettings.n_frame_threads = 1;
codec->internal->dav1dSettings.n_tile_threads = AVIF_CLAMP(decoder->maxThreads, 1, DAV1D_MAX_TILE_THREADS);
codec->internal->dav1dSettings.frame_size_limit = decoder->imageSizeLimit;
Copy link
Collaborator

@wantehchang wantehchang Feb 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to add a comment to note that dav1d uses the same convention that 0 = unlimited, so this simple assignment is safe.

Note: I found that dav1d/tools/dav1d_cli_parse.c disallows its --sizelimit argument to be exactly UINT_MAX:

        case ARG_SIZE_LIMIT: {
            char *arg = optarg, *end;
            uint64_t res = strtoul(arg, &end, 0);
            if (*end == 'x') // NxM
                res *= strtoul((arg = end + 1), &end, 0);
            if (*end || end == arg || res >= UINT_MAX)
                error(argv[0], optarg, ARG_SIZE_LIMIT, "an integer or dimension");
            lib_settings->frame_size_limit = (unsigned) res;
            break;
        }

I don't know whether that is intentional or an off-by-one bug.


if (dav1d_open(&codec->internal->dav1dContext, &codec->internal->dav1dSettings) != 0) {
return AVIF_FALSE;
Expand Down Expand Up @@ -209,9 +210,6 @@ avifCodec * avifCodecCreateDav1d(void)
memset(codec->internal, 0, sizeof(struct avifCodecInternal));
dav1d_default_settings(&codec->internal->dav1dSettings);

// Set a maximum frame size limit to avoid OOM'ing fuzzers.
codec->internal->dav1dSettings.frame_size_limit = AVIF_MAX_IMAGE_SIZE;

// Ensure that we only get the "highest spatial layer" as a single frame
// for each input sample, instead of getting each spatial layer as its own
// frame one at a time ("all layers").
Expand Down