Skip to content
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.

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

uncompressed: add sanity limit check on number of tile rows and cols #1207

Merged

Conversation

bradh
Copy link
Contributor

@bradh bradh commented Jun 28, 2024

Resolves #1206

Includes unit test.

This does not make the limit configurable, but that could be added.

Should we have the same limit for the number of image grid rows and columns? That already has a short-form and long-form option, but no limits on the long form case:

libheif/libheif/context.cc

Lines 189 to 212 in 7a8c58e

if (field_size == 32) {
if (data.size() < 12) {
return Error(heif_error_Invalid_input,
heif_suberror_Invalid_grid_data,
"Grid image data incomplete");
}
m_output_width = ((data[4] << 24) |
(data[5] << 16) |
(data[6] << 8) |
(data[7]));
m_output_height = ((data[8] << 24) |
(data[9] << 16) |
(data[10] << 8) |
(data[11]));
}
else {
m_output_width = ((data[4] << 8) |
(data[5]));
m_output_height = ((data[6] << 8) |
(data[7]));
}

@farindk
Copy link
Contributor

farindk commented Jun 28, 2024

We might be able to omit that limit for grid images (total image size) because with the planned ROI decoding, it would be perfectly fine to have a huge image above the security limit if each tile is below the limit.

@farindk
Copy link
Contributor

farindk commented Jun 28, 2024

Image Grid has a maximum of 256 tile rows/columns, stored in uint16.

@bradh
Copy link
Contributor Author

bradh commented Jun 28, 2024

Image Grid has a maximum of 256 tile rows/columns, stored in uint16.

Ah, indeed. Mixed up the image size and the tile limits.

@farindk
Copy link
Contributor

farindk commented Jul 4, 2024

I suggest that we limit the total number of tiles instead of width/height separately, as was recently implemented in #1204

I.e. MAX_NUMBER_OF_TILES = 32768*32768;

@bradh
Copy link
Contributor Author

bradh commented Jul 4, 2024

I suggest that we limit the total number of tiles instead of width/height separately, as was recently implemented in #1204

Sounds reasonable.

I.e. MAX_NUMBER_OF_TILES = 32768*32768;

Is this the limit value you want to use? Essentially allowing number of tiles to equal number of pixels?

@farindk
Copy link
Contributor

farindk commented Jul 4, 2024

I.e. MAX_NUMBER_OF_TILES = 32768*32768;

Is this the limit value you want to use? Essentially allowing number of tiles to equal number of pixels?

I just used your MAX_TILE_GRID_ROW_OR_COLUMN value. The question is probably whether we need any tile limit at all. AFAIK, there is no memory overhead in uncompressed codec for tiles and at some point, the image size limit kicks in anyway.

Thus: do we need a tile limit?

@bradh
Copy link
Contributor Author

bradh commented Jul 4, 2024

OK. So the plan is to limit number of uncompressed tile rows and tile columns to less than what can be represented in uint32_t (i.e. just avoiding the overflow in #1206), and not limiting grid. Then for really huge images, the user only has to increase the maximum image size limits, and we don't need to add API to change the tile size limits.

Does that seem reasonable?

@farindk
Copy link
Contributor

farindk commented Jul 5, 2024

Yes. And for huge images, the user does not even have to increase the limit if he is accessing the image only in small ROIs that are below the limit. (Through the ROI decoding API that I'm going to add.)

@bradh bradh force-pushed the uncompressed_tile_limits_2024-06-28 branch from 307755d to bd5748f Compare July 5, 2024 09:25
@bradh
Copy link
Contributor Author

bradh commented Jul 5, 2024

Have rebased. Should now match the intent above.

@farindk
Copy link
Contributor

farindk commented Jul 5, 2024

Looks good. Thank you.

@farindk farindk merged commit 32dc21b into strukturag:master Jul 5, 2024
34 of 35 checks passed
@bradh bradh deleted the uncompressed_tile_limits_2024-06-28 branch July 5, 2024 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integer overflow in uncompressed_box.cc
2 participants