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 all commits
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 should be tolerated.\n");
printf(" Default: %u, set to 0 to disable.\n", AVIF_DEFAULT_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_DEFAULT_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
13 changes: 12 additions & 1 deletion 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 reasonable default for maximum image size to avoid out-of-memory errors or integer overflow in
// (32-bit) int or unsigned int arithmetic operations.
#define AVIF_DEFAULT_MAX_IMAGE_SIZE (16384 * 16384)

enum avifPlanesFlags
{
AVIF_PLANES_YUV = (1 << 0),
Expand Down Expand Up @@ -140,7 +144,8 @@ typedef enum avifResult
AVIF_RESULT_IO_ERROR,
AVIF_RESULT_WAITING_ON_IO, // similar to EAGAIN/EWOULDBLOCK, this means the avifIO doesn't have necessary data available yet
AVIF_RESULT_INVALID_ARGUMENT, // an argument passed into this function is invalid
AVIF_RESULT_NOT_IMPLEMENTED // a requested code path is not (yet) implemented
AVIF_RESULT_NOT_IMPLEMENTED, // a requested code path is not (yet) implemented
AVIF_RESULT_IMAGE_TOO_LARGE // The image exceeds the configured imageSizeLimit
} avifResult;

AVIF_API const char * avifResultToString(avifResult result);
Expand Down Expand Up @@ -701,6 +706,12 @@ typedef struct avifDecoder
avifBool ignoreExif;
avifBool ignoreXMP;

// This represents the maximum size of a image (in pixel count) that libavif and the underlying
// AV1 decoder should attempt to decode. It defaults to AVIF_DEFAULT_MAX_IMAGE_SIZE, and can be
// set to 0 to disable the limit.
// Note: Only some underlying AV1 codecs support a configurable size limit (such as dav1d).
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
1 change: 1 addition & 0 deletions src/avif.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ const char * avifResultToString(avifResult result)
case AVIF_RESULT_WAITING_ON_IO: return "Waiting on IO";
case AVIF_RESULT_INVALID_ARGUMENT: return "Invalid argument";
case AVIF_RESULT_NOT_IMPLEMENTED: return "Not implemented";
case AVIF_RESULT_IMAGE_TOO_LARGE: return "Image too large";
case AVIF_RESULT_UNKNOWN_ERROR:
default:
break;
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
33 changes: 23 additions & 10 deletions src/read.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1182,14 +1182,14 @@ static avifBool avifParseItemLocationBox(avifMeta * meta, const uint8_t * raw, s
return AVIF_TRUE;
}

static avifBool avifParseImageGridBox(avifImageGrid * grid, const uint8_t * raw, size_t rawLen)
static avifResult avifParseImageGridBox(avifImageGrid * grid, const uint8_t * raw, size_t rawLen, uint32_t imageSizeLimit)
{
BEGIN_STREAM(s, raw, rawLen);

uint8_t version, flags;
CHECK(avifROStreamRead(&s, &version, 1)); // unsigned int(8) version = 0;
if (version != 0) {
return AVIF_FALSE;
return AVIF_RESULT_INVALID_IMAGE_GRID;
}
uint8_t rowsMinusOne, columnsMinusOne;
CHECK(avifROStreamRead(&s, &flags, 1)); // unsigned int(8) flags;
Expand All @@ -1208,15 +1208,18 @@ static avifBool avifParseImageGridBox(avifImageGrid * grid, const uint8_t * raw,
} else {
if (fieldLength != 32) {
// This should be impossible
return AVIF_FALSE;
return AVIF_RESULT_INVALID_IMAGE_GRID;
}
CHECK(avifROStreamReadU32(&s, &grid->outputWidth)); // unsigned int(FieldLength) output_width;
CHECK(avifROStreamReadU32(&s, &grid->outputHeight)); // unsigned int(FieldLength) output_height;
}
if ((grid->outputWidth == 0) || (grid->outputHeight == 0) || (grid->outputWidth > (AVIF_MAX_IMAGE_SIZE / grid->outputHeight))) {
return AVIF_FALSE;
if ((grid->outputWidth == 0) || (grid->outputHeight == 0) || (avifROStreamRemainingBytes(&s) != 0)) {
return AVIF_RESULT_INVALID_IMAGE_GRID;
}
if (imageSizeLimit && (grid->outputWidth > (imageSizeLimit / grid->outputHeight))) {
Copy link
Collaborator

@wantehchang wantehchang Mar 8, 2021

Choose a reason for hiding this comment

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

If imageSizeLimit is equal to 0, we still need to make sure grid->outputWidth * grid->outputHeight does not overflow size_t. We can either check that here or when we allocate the plane buffers.

return AVIF_RESULT_IMAGE_TOO_LARGE;
}
return avifROStreamRemainingBytes(&s) == 0;
return AVIF_RESULT_OK;
}

static avifBool avifParseImageSpatialExtentsProperty(avifProperty * prop, const uint8_t * raw, size_t rawLen)
Expand Down Expand Up @@ -2233,6 +2236,7 @@ avifDecoder * avifDecoderCreate(void)
avifDecoder * decoder = (avifDecoder *)avifAlloc(sizeof(avifDecoder));
memset(decoder, 0, sizeof(avifDecoder));
decoder->maxThreads = 1;
decoder->imageSizeLimit = AVIF_DEFAULT_MAX_IMAGE_SIZE;
return decoder;
}

Expand Down Expand Up @@ -2632,8 +2636,9 @@ avifResult avifDecoderReset(avifDecoder * decoder)
if (readResult != AVIF_RESULT_OK) {
return readResult;
}
if (!avifParseImageGridBox(&data->colorGrid, readData.data, readData.size)) {
return AVIF_RESULT_INVALID_IMAGE_GRID;
avifResult parseResult = avifParseImageGridBox(&data->colorGrid, readData.data, readData.size, decoder->imageSizeLimit);
if (parseResult != AVIF_RESULT_OK) {
return parseResult;
}
}

Expand Down Expand Up @@ -2671,8 +2676,10 @@ avifResult avifDecoderReset(avifDecoder * decoder)
if (readResult != AVIF_RESULT_OK) {
return readResult;
}
if (!avifParseImageGridBox(&data->alphaGrid, readData.data, readData.size)) {
return AVIF_RESULT_INVALID_IMAGE_GRID;
avifResult parseResult =
avifParseImageGridBox(&data->alphaGrid, readData.data, readData.size, decoder->imageSizeLimit);
if (parseResult != AVIF_RESULT_OK) {
return parseResult;
}
}

Expand Down Expand Up @@ -2747,6 +2754,12 @@ avifResult avifDecoderReset(avifDecoder * decoder)
if (ispeProp) {
decoder->image->width = ispeProp->u.ispe.width;
decoder->image->height = ispeProp->u.ispe.height;
if (!decoder->image->width || !decoder->image->height) {
return AVIF_RESULT_BMFF_PARSE_FAILED;
}
if (decoder->imageSizeLimit && (decoder->image->width > (decoder->imageSizeLimit / decoder->image->height))) {
return AVIF_RESULT_IMAGE_TOO_LARGE;
}
} else {
decoder->image->width = 0;
decoder->image->height = 0;
Expand Down
38 changes: 19 additions & 19 deletions src/reformat.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ avifResult avifImageRGBToYUV(avifImage * image, const avifRGBImage * rgb)
uint32_t * yuvRowBytes = image->yuvRowBytes;
for (uint32_t outerJ = 0; outerJ < image->height; outerJ += 2) {
for (uint32_t outerI = 0; outerI < image->width; outerI += 2) {
int blockW = 2, blockH = 2;
uint32_t blockW = 2, blockH = 2;
if ((outerI + 1) >= image->width) {
blockW = 1;
}
Expand All @@ -189,10 +189,10 @@ avifResult avifImageRGBToYUV(avifImage * image, const avifRGBImage * rgb)
}

// Convert an entire 2x2 block to YUV, and populate any fully sampled channels as we go
for (int bJ = 0; bJ < blockH; ++bJ) {
for (int bI = 0; bI < blockW; ++bI) {
int i = outerI + bI;
int j = outerJ + bJ;
for (uint32_t bJ = 0; bJ < blockH; ++bJ) {
for (uint32_t bI = 0; bI < blockW; ++bI) {
uint32_t i = outerI + bI;
uint32_t j = outerJ + bJ;

// Unpack RGB into normalized float
if (state.rgbChannelBytes > 1) {
Expand Down Expand Up @@ -297,8 +297,8 @@ avifResult avifImageRGBToYUV(avifImage * image, const avifRGBImage * rgb)

float sumU = 0.0f;
float sumV = 0.0f;
for (int bJ = 0; bJ < blockH; ++bJ) {
for (int bI = 0; bI < blockW; ++bI) {
for (uint32_t bJ = 0; bJ < blockH; ++bJ) {
for (uint32_t bI = 0; bI < blockW; ++bI) {
sumU += yuvBlock[bI][bJ].u;
sumV += yuvBlock[bI][bJ].v;
}
Expand All @@ -307,10 +307,10 @@ avifResult avifImageRGBToYUV(avifImage * image, const avifRGBImage * rgb)
float avgU = sumU / totalSamples;
float avgV = sumV / totalSamples;

const int chromaShiftX = 1;
const int chromaShiftY = 1;
int uvI = outerI >> chromaShiftX;
int uvJ = outerJ >> chromaShiftY;
const uint32_t chromaShiftX = 1;
const uint32_t chromaShiftY = 1;
uint32_t uvI = outerI >> chromaShiftX;
uint32_t uvJ = outerJ >> chromaShiftY;
Copy link
Collaborator

@wantehchang wantehchang Mar 8, 2021

Choose a reason for hiding this comment

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

Here we need to make sure the multiplication uvJ * yuvRowBytes[AVIF_CHAN_U] at line 315 below is done using the size_t type and also make sure that multiplication does not overflow size_t.

It is best to check for potential overflow on entry to this function.

By now it should be clear why we (and Chrome) set AVIF_MAX_IMAGE_SIZE to 2^28, so that we can perform these arithmetic operations using the 32-bit 'int' or 'uint32_t' type freely.

if (state.yuvChannelBytes > 1) {
uint16_t * pU = (uint16_t *)&yuvPlanes[AVIF_CHAN_U][(uvI * 2) + (uvJ * yuvRowBytes[AVIF_CHAN_U])];
*pU = (uint16_t)avifReformatStateUVToUNorm(&state, avgU);
Expand All @@ -323,20 +323,20 @@ avifResult avifImageRGBToYUV(avifImage * image, const avifRGBImage * rgb)
} else if (image->yuvFormat == AVIF_PIXEL_FORMAT_YUV422) {
// YUV422, average 2 samples (1x2), twice

for (int bJ = 0; bJ < blockH; ++bJ) {
for (uint32_t bJ = 0; bJ < blockH; ++bJ) {
float sumU = 0.0f;
float sumV = 0.0f;
for (int bI = 0; bI < blockW; ++bI) {
for (uint32_t bI = 0; bI < blockW; ++bI) {
sumU += yuvBlock[bI][bJ].u;
sumV += yuvBlock[bI][bJ].v;
}
float totalSamples = (float)blockW;
float avgU = sumU / totalSamples;
float avgV = sumV / totalSamples;

const int chromaShiftX = 1;
int uvI = outerI >> chromaShiftX;
int uvJ = outerJ + bJ;
const uint32_t chromaShiftX = 1;
uint32_t uvI = outerI >> chromaShiftX;
uint32_t uvJ = outerJ + bJ;
if (state.yuvChannelBytes > 1) {
uint16_t * pU = (uint16_t *)&yuvPlanes[AVIF_CHAN_U][(uvI * 2) + (uvJ * yuvRowBytes[AVIF_CHAN_U])];
*pU = (uint16_t)avifReformatStateUVToUNorm(&state, avgU);
Expand Down Expand Up @@ -488,7 +488,7 @@ static avifResult avifImageYUVAnyToRGBAnySlow(const avifImage * image,
uint16_t unormU[2][2], unormV[2][2];

// How many bytes to add to a uint8_t pointer index to get to the adjacent (lesser) sample in a given direction
int uAdjCol, vAdjCol, uAdjRow, vAdjRow;
uint32_t uAdjCol, vAdjCol, uAdjRow, vAdjRow;
if ((i == 0) || ((i == (image->width - 1)) && ((i % 2) != 0))) {
uAdjCol = 0;
vAdjCol = 0;
Expand Down Expand Up @@ -538,8 +538,8 @@ static avifResult avifImageYUVAnyToRGBAnySlow(const avifImage * image,
unormV[1][1] = *((const uint16_t *)&vPlane[(uvJ * vRowBytes) + (uvI * yuvChannelBytes) + vAdjCol + vAdjRow]);

// clamp incoming data to protect against bad LUT lookups
for (int bJ = 0; bJ < 2; ++bJ) {
for (int bI = 0; bI < 2; ++bI) {
for (uint32_t bJ = 0; bJ < 2; ++bJ) {
for (uint32_t bI = 0; bI < 2; ++bI) {
unormU[bI][bJ] = AVIF_MIN(unormU[bI][bJ], yuvMaxChannel);
unormV[bI][bJ] = AVIF_MIN(unormV[bI][bJ], yuvMaxChannel);
}
Expand Down