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

Image.verify raises struct.error (instead of IndexError) on Pillow > 2.7.0 #1755

Closed
melinath opened this issue Mar 2, 2016 · 8 comments · Fixed by #1805
Closed

Image.verify raises struct.error (instead of IndexError) on Pillow > 2.7.0 #1755

melinath opened this issue Mar 2, 2016 · 8 comments · Fixed by #1805

Comments

@melinath
Copy link

melinath commented Mar 2, 2016

Traceback (most recent call last):
  File "daguerre/tests/unit/test_adjustments.py", line 329, in test_adjust__broken
    self.assertRaises(IndexError, image.verify)
  File "unittest/case.py", line 475, in assertRaises
    callableObj(*args, **kwargs)
  File "PIL/PngImagePlugin.py", line 544, in verify
    self.png.verify()
  File "PIL/PngImagePlugin.py", line 163, in verify
    self.crc(cid, ImageFile._safe_read(self.fp, length))
  File "PIL/PngImagePlugin.py", line 142, in crc
    crc2 = i16(self.fp.read(2)), i16(self.fp.read(2))
  File "PIL/_binary.py", line 53, in i16be
    return unpack(">H", c[o:o+2])[0]
error: unpack requires a string argument of length 2

Similar to #1163 in form, but maybe not in cause.

@melinath
Copy link
Author

melinath commented Mar 2, 2016

Here's the image I'm trying to verify:

broken

@melinath melinath changed the title Image.verify raises struct.error (instead of IOError) on Pillow 3.1.1 Image.verify raises struct.error (instead of... IndexError?) on Pillow 3.1.1 Mar 2, 2016
@melinath
Copy link
Author

melinath commented Mar 2, 2016

Verified that this test (for IndexError) works on 2.3.0, the last version I was testing with. I'll check a few more versions to try to narrow it down.

@melinath melinath changed the title Image.verify raises struct.error (instead of... IndexError?) on Pillow 3.1.1 Image.verify raises struct.error (instead of... IndexError) on Pillow 3.1.1 Mar 2, 2016
@melinath
Copy link
Author

melinath commented Mar 2, 2016

2.9.0: Broken
2.8.2: Broken
2.7.0: Works

@melinath melinath changed the title Image.verify raises struct.error (instead of... IndexError) on Pillow 3.1.1 Image.verify raises struct.error (instead of... IndexError) on Pillow > 2.7.0 Mar 2, 2016
@melinath melinath changed the title Image.verify raises struct.error (instead of... IndexError) on Pillow > 2.7.0 Image.verify raises struct.error (instead of IndexError) on Pillow > 2.7.0 Mar 2, 2016
@wiredfool
Copy link
Member

The underlying implementation of the binary functions changed to struct.unpack at about 2.7 and every time that we throw a struct.error it's because of that change.

@hugovk
Copy link
Member

hugovk commented Mar 2, 2016

That change was in PR #1121, which was released in 2.8.0.

@radarhere
Copy link
Member

Given that you've created a PR in another project to act on this, are you right for this issue to be closed?

@melinath
Copy link
Author

melinath commented Mar 3, 2016

I mean... I need to support both in any case, so I don't care. The question is more what you feel is a more appropriate error as far as I'm concerned. :-) I'm guessing struct.error might make more sense? But OTOH I personally wouldn't have known that was a struct.error if you hadn't told me, which makes the error message pretty opaque.

But yeah, I personally don't need anything from you on this.

@wiredfool
Copy link
Member

We've treated this as an error for opening files, it makes sense to do it for verify. (though, verify is a very inconsistently supported function, currently it's only available for pngs)

@wiredfool wiredfool added this to the 3.2.0 milestone Mar 14, 2016
@radarhere radarhere removed this from the 3.2.0 milestone Apr 2, 2016
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 a pull request may close this issue.

4 participants