-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Fix DDS Timeout issues in oss-fuzz #9404
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
base: main
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
| "Tests/images/timeout-c60a3d7314213624607bfb3e38d551a8b24a7435.dds", | ||
| ], | ||
| ) | ||
| def test_timeout(test_file) -> None: |
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.
| def test_timeout(test_file) -> None: | |
| def test_timeout(test_file: str) -> None: |
| data += o8(int((masked_value >> mask_offsets[i]) * mask_totals[i])) | ||
|
|
||
| # extra padding pixels -- always all 0 | ||
| if len(data) < dest_length: |
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.
If the file stopped before we were able to read all the bytes we needed, why not just let the data be shorter than expected, leading to a ValueError: not enough image data be raised? That error sounds like it is accurately describing the situation.
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.
Good question - I couldn't tell if it was intentional that it filled off the end. Given that it's plain black, it's at least a reasonable fill, but I didn't see a spec. I'd be happy enough erroring out as well. Main issue here is that I want to catch things other than dds timeouts in oss-fuzz.
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've created #9405
| while len(data) < dest_length: | ||
| value = int.from_bytes(self.fd.read(bytecount), "little") | ||
| # consume the data | ||
| has_more = True |
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.
| has_more = True | |
| has_more = bytecount != 0 |
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.
Bytecount is number of bytes per pixel, or the number of masks. It's not the data size.
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.
Yes, but if bytecount is zero, then self.fd.read(bytecount) is always going to be empty.
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.
But bytecount should never be zero. It’s an image level compression property, how many bytes do we consume per pixel.
To me, this looks like a misunderstanding, so can you explain why it’s better to initialize with this rather than the simpler expression for true?
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.
But bytecount should never be zero
...awkwardly, it does seem to be possible. I mean, it doesn't seem helpful value, and I do think it should lead to an error.
Pillow/src/PIL/DdsImagePlugin.py
Lines 355 to 356 in bc64ccb
| # pixel format | |
| pfsize, pfflags, fourcc, bitcount = struct.unpack("<4I", header[68:84]) |
Pillow/src/PIL/DdsImagePlugin.py
Line 509 in bc64ccb
| bytecount = bitcount // 8 |
Here's the documentation - https://learn.microsoft.com/en-us/windows/win32/direct3ddds/dds-pixelformat
dwRGBBitCount
Type: DWORD
Number of bits in an RGB (possibly including alpha) format. Valid when dwFlags includes DDPF_RGB, DDPF_LUMINANCE, or DDPF_YUV.
To demonstrate, I've added radarhere@c90d124 onto a fork of this branch - https://github.com/radarhere/Pillow/actions/runs/21312659863/job/61351156707#step:11:58
bitcount is set to 0 in _open
bitcount is 0 which means that bytecount is 0 within DdsRgbDecoder
A recent run of oss-fuzz here ended up entirely finding dds parser timeout issues. There were 40 or so of them.
This PR has two significant changes:
Hoist a divide + comparison out of the hot loop. Using the line profiler, this nets us about 10% on the inner loop. Not great, but not bad though.
Only go pixel by pixel for the data we have in the file, and bulk fill with 0 once we hit the end. This gets us the 500x speed increase we need, but this only really helps semi-invalid images where the file data is much smaller than the image data that we're generating. We probably won't hit this on real images, but fuzzed ones hit it constantly. On the downside, we give up the first 10% gain in the hot loop checking to see if we read any data.
initial lineperf:
Premultiplied:
And the final config: