-
Notifications
You must be signed in to change notification settings - Fork 590
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
Conversation
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 <[email protected]>
There was a problem hiding this 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.
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. |
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. |
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. |
@jessey-git Yeah, for ImageOutput, there's a |
There was a problem hiding this 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 <[email protected]>
…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 <[email protected]>
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.