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 unpack to c_ssize_t on 32 bit systems with corrupt EXIF data #3995

Closed
wants to merge 9 commits into from

Conversation

@paddywwoof
Copy link
Contributor

commented Jul 30, 2019

Add an additional specific try except to allow continued iteration through tags and image data to be subsequently read. On 64bit systems this isn't an issue as 4 bytes can't be unpacked to overflow ssize_t.

The alternative approach would have been to mask the value of offset like:

offset, = self._unpack("L", data)
offset = offset & ((1 < 31) - 1)

but that would prevent offset ever taking a negative value which might be bad

@radarhere radarhere changed the title fix unpack to c_ssize_t on 32 bit systems with corrupt EXIF data Fix unpack to c_ssize_t on 32 bit systems with corrupt EXIF data Jul 30, 2019

@radarhere

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

Would you be able to add a test? One that fails without your fix and passes with it.

paddywwoof added some commits Aug 4, 2019

@paddywwoof

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

Impressive amount of checking, if maybe a little pedantic! However I'm not sure how the issue targeted by this pull request will be verified as it needs to be run on a 32 bit system. Is this done manually prior to each release?

Paddy

@radarhere

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

Half of the AppVeyor jobs are 32-bit, so it should already be tested.

@paddywwoof

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

Great. I'm not sure what the appveyor test failure is (other pull requests also seem to be suffering from it) - presumably relates to #4019. Let me know if there's anything I can do to help or test.

@hugovk

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

#4019 is now merged, if you rebase or merge from master it should now pass.

paddywwoof added some commits Aug 11, 2019

@radarhere

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

If I print out the tags and the offsets at the relevant line,

diff --git a/src/PIL/TiffImagePlugin.py b/src/PIL/TiffImagePlugin.py
index dabcf8eb4..c6009a036 100644
--- a/src/PIL/TiffImagePlugin.py
+++ b/src/PIL/TiffImagePlugin.py
@@ -771,6 +771,7 @@ class ImageFileDirectory_v2(MutableMapping):
                             "Tag Location: %s - Data Location: %s" % (here, offset),
                             end=" ",
                         )
+                    print("Tag "+str(tag)+": Offset "+str(offset))
                     fp.seek(offset)
                     data = ImageFile._safe_read(fp, size)
                     fp.seek(here)

I get

Tag 271: Offset 8
Tag 272: Offset 26
Tag 282: Offset 38
Tag 283: Offset 46
Tag 305: Offset 54
Tag 306: Offset 96
Tag 33432: Offset 116
Tag 33434: Offset 138
Tag 33437: Offset 146
Tag 36867: Offset 154
Tag 36868: Offset 174
Tag 37377: Offset 194
Tag 37378: Offset 202
Tag 37380: Offset 210
Tag 37381: Offset 218
Tag 37386: Offset 226
Tag 41988: Offset 234
Tag 42033: Offset 242
Tag 42036: Offset 250
Tag 2: Offset 280
Tag 4: Offset 304

This looks like a problem with the test image to me - at least one of the offset values should be greater than 32-bit to trigger the error, yes?

@paddywwoof

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

Andrew, this is my mistake. The latest version of Pillow does generate the tags you list, the version I tested the python mods on produced these tags from the same file:

Tag 0: Offset 65536
Tag 0: Offset 268493055
Tag 4352: Offset 33563137
Tag 221: Offset 66316
Tag 55807: Offset 18
Tag 5: Offset 2191327232
...

If the fixes to the exif reading and associated tests can guarantee that no big offsets will get through then my modification is superfluous and can be rejected.

@radarhere

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

What version of Pillow are you testing with?

@radarhere

This comment has been minimized.

Copy link
Member

commented Aug 24, 2019

Testing, I find that this problem - #3994 - is fixed by #3584, in Pillow 6.0. See also the issue behind that PR, #3337

So, your image can be read correctly by current Pillow. However, I think it's because the image is actually fine - meaning that the fix does not address any concerns you might have about 32-bit systems.

@paddywwoof

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2019

@radarhere , yes. The version I was using and testing on was the 'standard' Pillow packaged with debian buster as raspbian for Raspberry Pi (still version 5.1). I intend the module I maintain (pi3d) to be easy for user of the vanilla system without having to upgrade PIL so I was reluctant to do that on the system I was testing on. However on a new setup I have tested the latest PIL and, as you say, the #3584 fix has prevented the corrupt offsets that caused the problem. If the exif corruption isn't going to happen in future then my suggested mod is not needed and I'm happy for it to be nullified.

Do I need to do that or can you, as a Pillow member?

@radarhere

This comment has been minimized.

Copy link
Member

commented Aug 24, 2019

I can do that. Thanks for your help in discussing the problem

@radarhere radarhere closed this Aug 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.