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

fix: various protections against corrupted files #3954

Merged
merged 2 commits into from
Aug 23, 2023

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Aug 22, 2023

Fixes #3952

Several interrelated fixes to guard against corrupted files:

  • Extend the limits:imagesize_MB sanity check to Targa files to address AddressSanitizer: allocator is out of memory #3952 specifically, and check in that failing example file into the testsuite. (N.B.: Only a couple readers use this, TIFF and now Targa. it should be extended to the others. Mackerel!)

  • Improve the error message used for the above, both for Targa and TIFF.

  • For low-memory situations, perhaps the default of 32GB is too large. Make the default be the minimum of 32GB and 1/2 the size of physical memory on the machine. (An app that legit expects humongous files is expected to raise the limit appropriately.)

  • The generic "Read error: hit end of file" we sometimes issued from ImageInput::ioread() was hard to discern which reader it came from, in cases where the file extension was absent or misleading and a succession of files were tried. By changing this error message to say which format reader triggered the error, it might make these errors easier to track down in the future. This also changed the reference output of a few test involving truncated or corrupted files.

Fixes 3952

Several interrelated fixes to guard against corrupted files:

* Extend the `limits:max_imagesize_MB` sanity check to Targa files to
  address 3952 specifically, and check in that failing example file
  into the testsuite. (N.B.: Only a couple readers use this, TIFF and
  now Targa. it should be extended to the others. Mackerel!)

* Improve the error message used for the above, both for Targa and
  TIFF.

* For low-memory situations, perhaps the default of 32GB is too large.
  Make the default be the minimum of 32GB and 1/2 the size of physical
  memory on the machine. (An app that legit expects humongous files is
  expected to raise the limit appropriately.)

* The generic "Read error: hit end of file" we sometimes issued from
  `ImageInput::ioread()` was hard to discern which reader it came
  from, in cases where the file extension was absent or misleading and
  a succession of files were tried. By changing this error message to
  say which format reader triggered the error, it might make these
  errors easier to track down in the future. This also changed the
  reference output of a few test involving truncated or corrupted
  files.

Signed-off-by: Larry Gritz <lg@larrygritz.com>
Copy link
Collaborator

@ThiagoIze ThiagoIze left a comment

Choose a reason for hiding this comment

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

I like the more informative error messages and extra checking. As I mentioned in #3952 (comment), I'm not sold on half the physical memory limitation since some users might be running with older machines (laptops?) that don't have that much memory.

@lgritz
Copy link
Collaborator Author

lgritz commented Aug 22, 2023

Yeah, this is just a proposal. If we don't like the physical_memory trick, we don't have to do it. But I want to find a way to address the fuzz testing with the sanitizer enabled, which when it crashes will be reported and we'll need to investigate with high priority under threat of CVEs being filed.

@lgritz
Copy link
Collaborator Author

lgritz commented Aug 22, 2023

Would it be bad manners to use the min(physical_mem) trick only if the sanitizer is running? Can we even detect that? Feels kinda dirty, I don't want to mask legit bugs in a way that they won't be detected properly by the sanitizer. But I also don't want the sanitizer catching false positives that aren't actually problems.

@jessey-git
Copy link
Contributor

For the targa code check, and thinking ahead to the other formats, is there a way to extract the check+error message to some place common? Instead of several lines of boilerplate added to each format, only a one line call to the common method would be needed.

Tough call on the default size really. I can't come up with a reasonable value to satisfy both the fuzzing and small-device scenarios.

@lgritz
Copy link
Collaborator Author

lgritz commented Aug 22, 2023

@jessey-git Yeah, for ImageOutput, there's a ImageOutput::check_open() method that provides a central place to do validity checks on a number of things. We could do something similar for ImageInput to centralize these checks. I'd like to do that in a separate PR, though, it's likely to be more intrusive.

Copy link
Collaborator

@ThiagoIze ThiagoIze left a comment

Choose a reason for hiding this comment

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

OK, you have me convinced that most users with small amounts of RAM are unlikely to be operating on giant images.

Signed-off-by: Larry Gritz <lg@larrygritz.com>
@lgritz lgritz merged commit 8f423c9 into AcademySoftwareFoundation:master Aug 23, 2023
23 checks passed
@lgritz lgritz deleted the lg-mem branch August 25, 2023 01:08
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Aug 25, 2023
…dation#3954)

Fixes AcademySoftwareFoundation#3952

Several interrelated fixes to guard against corrupted files:

* Extend the `limits:imagesize_MB` sanity check to Targa files to
address AcademySoftwareFoundation#3952 specifically, and check in that failing example file into
the testsuite. (N.B.: Only a couple readers use this, TIFF and now
Targa. it should be extended to the others. Mackerel!)

* Improve the error message used for the above, both for Targa and TIFF.

* For low-memory situations, perhaps the default of 32GB is too large.
Make the default be the minimum of 32GB and 1/2 the size of physical
memory on the machine. (An app that legit expects humongous files is
expected to raise the limit appropriately.)

* The generic "Read error: hit end of file" we sometimes issued from
`ImageInput::ioread()` was hard to discern which reader it came from, in
cases where the file extension was absent or misleading and a succession
of files were tried. By changing this error message to say which format
reader triggered the error, it might make these errors easier to track
down in the future. This also changed the reference output of a few test
involving truncated or corrupted files.

---------

Signed-off-by: Larry Gritz <lg@larrygritz.com>
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.

AddressSanitizer: allocator is out of memory
3 participants