patch _imagingcms.c: use strncpy#153
Conversation
WalkthroughThe change updates the copying of mode strings within the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| strncpy(self->mode_in, mode_in, 8); | ||
| strncpy(self->mode_out, mode_out, 8); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
strncpy here can leave mode strings unterminated; ensure explicit NUL-termination
strncpy does not NUL-terminate when the source length ≥ n. For 8-char modes like "RGBA;16B", this yields an unterminated buffer and PyUnicode_FromString(self->mode_in/out) may read past the array, causing OOB reads/UB. Copy up to sizeof(buf)-1 and force-terminate.
Apply this minimal, safe fix:
- strncpy(self->mode_in, mode_in, 8);
- strncpy(self->mode_out, mode_out, 8);
+ strncpy(self->mode_in, mode_in, sizeof(self->mode_in) - 1);
+ self->mode_in[sizeof(self->mode_in) - 1] = '\0';
+ strncpy(self->mode_out, mode_out, sizeof(self->mode_out) - 1);
+ self->mode_out[sizeof(self->mode_out) - 1] = '\0';Optional follow-ups (pick one, if you want to avoid truncating 8-char modes):
- Keep arrays as-is and change getters to not rely on a trailing NUL:
- inputMode getter: return PyUnicode_FromStringAndSize(self->mode_in, strnlen(self->mode_in, sizeof(self->mode_in)));
- outputMode getter: same pattern.
- Or increase buffers to 9 and keep the above termination pattern to preserve all 8 chars plus '\0' (verify ABI considerations; these structs are internal, but confirm downstream assumptions).
🏁 Script executed:
#!/bin/bash
set -e
echo "---- struct definition around mode_in/mode_out ----"
rg -n "mode_in" -A3 -B3 src/_imagingcms.c || true
rg -n "mode_out" -A3 -B3 src/_imagingcms.c || true
echo "---- strncpy usages ----"
rg -n -C2 "strncpy" src/_imagingcms.c || true
echo "---- Getter functions using mode_in/mode_out ----"
rg -n "PyUnicode_FromString" -C3 src/_imagingcms.c || trueLength of output: 3227
Ensure mode_in/mode_out are always NUL-terminated
strncpy(self->mode_in, mode_in, 8) and likewise for mode_out won’t append '\0' if the source is ≥ 8 bytes (e.g. "RGBA;16B"). Since the getters call PyUnicode_FromString(), a missing terminator can lead to OOB reads and undefined behavior.
• File: src/_imagingcms.c
Lines 204–205 (inside cms_transform_new): replace the two strncpy calls with:
- strncpy(self->mode_in, mode_in, 8);
- strncpy(self->mode_out, mode_out, 8);
+ strncpy(self->mode_in, mode_in, sizeof(self->mode_in) - 1);
+ self->mode_in[sizeof(self->mode_in) - 1] = '\0';
+ strncpy(self->mode_out, mode_out, sizeof(self->mode_out) - 1);
+ self->mode_out[sizeof(self->mode_out) - 1] = '\0';Optional follow-ups (choose one):
- Change the getters to use
PyUnicode_FromStringAndSize()withstrnlen()to avoid relying on a terminator. - Increase the buffer sizes to 9 to allow full 8-char modes plus
'\0'(verify ABI impact).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| strncpy(self->mode_in, mode_in, 8); | |
| strncpy(self->mode_out, mode_out, 8); | |
| /* ensure mode_in/mode_out are always NUL-terminated */ | |
| strncpy(self->mode_in, mode_in, sizeof(self->mode_in) - 1); | |
| self->mode_in[sizeof(self->mode_in) - 1] = '\0'; | |
| strncpy(self->mode_out, mode_out, sizeof(self->mode_out) - 1); | |
| self->mode_out[sizeof(self->mode_out) - 1] = '\0'; |
🤖 Prompt for AI Agents
In src/_imagingcms.c at lines 204-205 inside cms_transform_new, the strncpy
calls copying mode_in and mode_out do not guarantee NUL-termination if the
source strings are 8 or more characters, risking out-of-bounds reads. Replace
these strncpy calls by copying up to 7 characters and explicitly setting the 8th
byte to '\0' to ensure proper NUL-termination of the mode_in and mode_out
buffers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Tests/test_imagecms.py (2)
629-631: Strengthen the test to assert truncation behavior and prevent silent regressionsRight now this is a pure smoke test. Let’s minimally assert that a transform is created and the mode metadata is bounded to 8 chars, which is the intent of the strncpy fix for CVE-2024-28219.
def test_long_modes() -> None: - p = ImageCms.getOpenProfile("Tests/icc/sGrey-v2-nano.icc") - ImageCms.buildTransform(p, p, "ABCDEFGHI", "ABCDEFGHI") + """Regression for CVE-2024-28219: long mode strings should be safely handled.""" + p = ImageCms.getOpenProfile("Tests/icc/sGrey-v2-nano.icc") + t = ImageCms.buildTransform(p, p, "ABCDEFGHI", "ABCDEFGHI") + # Smoke check: transform is created and mode metadata is bounded + assert t is not None + assert hasattr(t, "inputMode") and hasattr(t, "outputMode") + assert len(t.inputMode) <= 8 and len(t.outputMode) <= 8 + # Optionally, if we want to pin truncation semantics exactly: + # assert t.inputMode == "ABCDEFGH" and t.outputMode == "ABCDEFGH"
629-631: Optional: Add boundary-case coverage (7, 8, 9, very long, non-ASCII)To harden against future regressions, consider a parametrized test to cover lengths around the boundary and non-ASCII input. This complements the smoke test without changing its intent.
Additional test to append elsewhere in this file:
import pytest @pytest.mark.parametrize("mode", [ "ABCDEFG", # 7 "ABCDEFGH", # 8 (exact fit) "ABCDEFGHI", # 9 (truncation) "A" * 1024, # very long "ĀBĆĐĖFGHI", # non-ASCII (ensure UTF-8 handling; will be truncated) ]) def test_long_modes_boundary(mode): from PIL import ImageCms p = ImageCms.getOpenProfile("Tests/icc/sGrey-v2-nano.icc") t = ImageCms.buildTransform(p, p, mode, mode) assert t is not None # Validate metadata is bounded to 8 characters regardless of input length assert hasattr(t, "inputMode") and hasattr(t, "outputMode") assert len(t.inputMode) <= 8 and len(t.outputMode) <= 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Tests/test_imagecms.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
Tests/test_imagecms.py (1)
src/PIL/ImageCms.py (1)
getOpenProfile(411-430)
2 participants
Fixes # CVE-2024-28219.
Changes proposed in this pull request:
Copying upstream changes to fix CVE-2024-28219. https://github.com/python-pillow/Pillow/pull/7928/files
Summary by CodeRabbit