Skip to content

Do not store malformed PhotometricInterpretation tag data - #10274

Open
saintparish4 wants to merge 2 commits into
python-pillow:mainfrom
saintparish4:tiff-photometric-keyerror
Open

saintparish4 wants to merge 2 commits into
python-pillow:mainfrom
saintparish4:tiff-photometric-keyerror

Conversation

@saintparish4

Copy link
Copy Markdown

Regression from #10210

#10210 started ignoring a PhotometricInterpretation tag with more than one value, by not setting it in _setitem(). However, the raw data is still kept in _tagdata, so the tag is reported by in and when iterating, but KeyError: 262 is raised when it is read.

This means that an image with such a tag can be opened and loaded, but reading its EXIF data or saving it as a TIFF fails.

import io
from PIL import Image

buf = io.BytesIO()
Image.new("L", (4, 4)).save(buf, "TIFF")
b = bytearray(buf.getvalue())
offset = int.from_bytes(b[4:8], "little")
for i in range(int.from_bytes(b[offset : offset + 2], "little")):
    entry = offset + 2 + 12 * i
    if int.from_bytes(b[entry : entry + 2], "little") == 262:
        # Change PhotometricInterpretation to have two values
        b[entry + 4 : entry + 12] = b"\x02\x00\x00\x00\x01\x00\x02\x00"

with Image.open(io.BytesIO(b)) as im:
    im.load()
    im.save(io.BytesIO(), "TIFF")  # KeyError: 262

Changes proposed in this pull request:

  • Skip a PhotometricInterpretation tag without exactly one value when loading the IFD, so that its data is not stored. This is only done for the main IFD, as tag 262 does not have a fixed length in the Exif, GPS and Interop IFDs.
  • Added a test.

If a malformed PhotometricInterpretation tag is followed by a valid duplicate, the valid one is now used, whereas after #10210 neither was. Let me know if you would prefer to keep ignoring both.

@radarhere radarhere added the TIFF label Oct 22, 2026
and count != 1
and self.group is None
):
# Ignore a malformed PhotometricInterpretation tag, like libtiff

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

like libtiff

I'm intrigued. Could you expand on how you know this, link to source code, or something?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It's from libtiff's directory reading. PhotometricInterpretation is defined as a single SHORT (tif_dirinfo.c#L104) and has no special case in TIFFReadDirectory, so it's read by TIFFFetchNormalTag(tif, dp, TRUE) (tif_dirread.c#L4896-4897). That calls TIFFReadDirEntryShort, which returns TIFFReadDirEntryErrCount if the count isn't 1 (#L485-489), so TIFFSetField is never called (#L6517-6529). Because recover is set, it only warns, Incorrect count for "PhotometricInterpretation"; tag ignored (#L4063-4067).

Looking into this also answered my question about duplicates. libtiff marks every later duplicate of a tag as ignored before reading any values (#L4389-4408), so a malformed PhotometricInterpretation followed by a valid one ends up with neither. I've pushed a commit so that Pillow does the same, with a test.

This only covers PhotometricInterpretation, though. When Pillow skips the first copy of another tag (unsupported type, empty data, etc.), a later duplicate is still used, unlike libtiff. I'm happy to make that general, here or in a follow-up, if you'd prefer.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants