Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions Tests/images/custom_gimp_palette.gpl
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
GIMP Palette
Name: custompalette
Columns: 4
#
0 0 0 Index 3
65 38 30 Index 4
103 62 49 Index 6
79 73 72 Index 7
114 101 97 Index 8
208 127 100 Index 9
151 144 142 Index 10
221 207 199 Index 11
# Original written by David Wetz in https://stackoverflow.com/questions/815836/im-creating-a-program-that-generates-a-palette-from-a-true-color-image-need-hel/815855#815855
Copy link
Member

Choose a reason for hiding this comment

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

Just an explanation of what is happening here - this isn't a random attribution. You're creating a comment that is over 100 characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

comments with >100 char lines long should not "break" the file. Even if we are reformatting here, the URL alone is wider than 100 chars in this case, so even breaking this in a functional way would still have lines wider than that.

0 0 0 Index 0
65 38 30 Index 1
103 62 49 Index 2
79 73 72 Index 3
114 101 97 Index 4
208 127 100 Index 5
151 144 142 Index 6
221 207 199 Index 7
20 changes: 20 additions & 0 deletions Tests/test_file_gimppalette.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,13 @@ def test_sanity() -> None:
GimpPaletteFile(fp)


def test_large_file_is_truncated(monkeypatch: pytest.MonkeyPatch) -> None:
monkeypatch.setattr(GimpPaletteFile, "_max_file_size", 100)
with open("Tests/images/custom_gimp_palette.gpl", "rb") as fp:
with pytest.warns(UserWarning):
GimpPaletteFile(fp)


def test_get_palette() -> None:
# Arrange
with open("Tests/images/custom_gimp_palette.gpl", "rb") as fp:
Expand All @@ -31,5 +38,18 @@ def test_get_palette() -> None:
palette, mode = palette_file.getpalette()

# Assert
expected_palette: list[int] = []
for color in (
(0, 0, 0),
(65, 38, 30),
(103, 62, 49),
(79, 73, 72),
(114, 101, 97),
(208, 127, 100),
(151, 144, 142),
(221, 207, 199),
):
expected_palette += color
assert palette == bytes(expected_palette)
assert mode == "RGB"
assert len(palette) / 3 == 8
23 changes: 20 additions & 3 deletions src/PIL/GimpPaletteFile.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
from __future__ import annotations

import re
import warnings
from typing import IO


Expand All @@ -24,24 +25,40 @@ class GimpPaletteFile:

rawmode = "RGB"

#: override if reading larger palettes is needed
max_colors = 256
_max_line_size = 100
Copy link
Member

Choose a reason for hiding this comment

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

We're reading RGB data from each line. That's three integers. Under what circumstances would a user need to read more than 100 characters?

Copy link
Contributor Author

@jsbueno jsbueno Oct 7, 2022

Choose a reason for hiding this comment

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

The 4th Column contains the color name, though that is currently unused. And there might be a name with more than 100 characters - moving the number to an attribute just offers a chance of one working around an error imposed by an arbitrary, though reasonable, limit on an otherwise good file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(currently unused in PIL, that is: GIMP uses that name)

_max_file_size = 2**20 # 1MB

def __init__(self, fp: IO[bytes]) -> None:
if not fp.readline().startswith(b"GIMP Palette"):
msg = "not a GIMP palette file"
raise SyntaxError(msg)

read = 0

palette: list[int] = []
for _ in range(256):
s = fp.readline()
while len(palette) < 3 * self.max_colors:

s = fp.readline(self._max_file_size)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
s = fp.readline(self._max_file_size)
s = fp.readline(self._max_line_size)

Copy link
Member

Choose a reason for hiding this comment

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

This looks like a mistake, where you've accidentally used the file size limit for the line size limit for the size argument.

Copy link
Contributor Author

@jsbueno jsbueno Oct 16, 2022

Choose a reason for hiding this comment

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

no - "file" size is deliberate here - to avoid DoS only.
line size checks are made just below, and as far as I know it is something just PIL has - there is no spec for this kind of file, but for GIMP source code, and there is no line size limit there.

Changing here for reading linesize would make the file break with long comments (as it did), and that makes no sense:

if one has a python project in which Palettes with this format are used, there is no reason to impose an arbitrary limit on comments on that source code. Resilience against malicious files when those are input by 3rd parties are mitigated through the file-size check.

if not s:
break

read += len(s)
if read >= self._max_file_size:
warnings.warn(
f"Palette file truncated at {self._max_file_size - len(s)} bytes"
)
break

# skip fields and comment lines
if re.match(rb"\w+:|#", s):
continue
if len(s) > 100:
if len(s) > self._max_line_size:
msg = "bad palette file"
raise SyntaxError(msg)

# 4th column is color name and may contain spaces.
v = s.split(maxsplit=3)
if len(v) < 3:
msg = "bad palette entry"
Expand Down
Loading