Repository navigation
ENG-1549 Bound parser memory on malformed input (v1.1.1-travv.1) - #1
Merged
Merged
Conversation
Add (*dicomio.Reader).Available and call it before every allocation sized from a value length (strings, OB/OW/UN values, encapsulated items, the unprocessed and error PixelData buffers), so a length larger than the input returns ErrorInsufficientBytesLeft instead of reserving that memory. Int and float values start from a capped capacity. A deflated input's limit becomes the most its compressed bytes can inflate to instead of no limit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CsCKfu9UiDrrq3xRqdtRmr
Rows, Columns, BitsAllocated, SamplesPerPixel and NumberOfFrames are read without MustGet*; a wrong type or missing value returns ErrorInvalidPixelDimensions instead of panicking. The expected length is computed in 64 bits, saturating instead of wrapping (a negative dimension saturates), so a product of declared dimensions can no longer wrap around to match vl; zero and negative dimensions fail the size check like any other mismatch, so AllowMismatchPixelDataLength still applies to them. Zero-byte frames are refused, so NumberOfFrames alone can no longer size the frame slice, and the frame buffers are only allocated once vl is known to be available. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CsCKfu9UiDrrq3xRqdtRmr
Add the MaxSequenceDepth parse option (default 64). Sequences and items count their nesting before reading what they contain, so a deeper input returns ErrorSequenceTooDeep instead of recursing until the stack overflows. An error inside a sequence or item now passes up unwrapped and only the top-level element wraps it: wrapped at every level, a message that quotes an element was copied once per level, and the strings grew with the square of the depth. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CsCKfu9UiDrrq3xRqdtRmr
A TransferSyntaxUID without a string value or with an unknown UID, an empty MetaElementGroupLength, a SpecificCharacterSet of the wrong type and a non-item element inside a defined-length sequence now return errors (or, for the character set, fall back to the default) instead of panicking. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CsCKfu9UiDrrq3xRqdtRmr
bounds_test.go parses small inputs whose declared lengths, PixelData dimensions or sequence nesting exceed what they hold, through every guarded site, and checks each returns its error within a small allocation; files nested up to the limit still parse, and the testdata files parse to the same JSON as upstream v1.1.0. FuzzParse checks that parsing never panics and allocates at most a fixed amount plus a multiple of the input size, seeded from those inputs, the small testdata files, and the .dcm files in $DICOM_VENDOR_SEEDS when set. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CsCKfu9UiDrrq3xRqdtRmr
Available only checks a declared length against a limit, which says nothing under LimitReadUntilEOF or on deflated input (1032 times the compressed bytes left). dicomio.Reader.ReadBytes now allocates once after Available only when the limit is exact (a caller-supplied size, not deflated); otherwise the buffer grows as bytes arrive. Strings, OB/OW/UN, encapsulated fragments, unprocessed and error PixelData read through it, and native frames read their bytes first under such a limit. readInt and readFloat drop their Available check: PushLimit already refuses, now with ErrorInsufficientBytesLeft, and their starting capacity is capped only when the limit is not exact. readNativeFrames refuses frames whose per-frame overhead (about 150 bytes: Frame, NativeFrame, slot, rounding) totals more than the PixelData length plus 64 KiB, so a native image allocates at most about twice its length (65 times for 1-bit pixels, unpacked to ints). One-byte frames cost 120 to 185 times their input before. Non-item elements in a sequence are named by tag, VR and length instead of printed whole, and no longer logged. A TransferSyntaxUID under 1.2.840.10008.1.2. missing from the UID list (HTJ2K, JPEG XL) is read as explicit VR little endian. A SpecificCharacterSet that is not a string is an error unless AllowUnknownSpecificCharacterSet is set. FuzzParse bounds every input, deflated ones included, by 80 bytes per byte read (deflated inputs count what they inflate to), and also runs ParseUntilEOF. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxyUkC8uMy3gASkRhe2hMu
inflatedSize assumed a UL group length; the parser also takes a US one or walks the group 0002 elements, and an input doing either got no allowance for what it inflates to. It now takes the larger of both ends, and a seed with a US group length covers it. Also: the SpecificCharacterSet type error names the Go type, OW skips the no-op swap on little endian input, and native frames check Available only where ReadBytes does not. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxyUkC8uMy3gASkRhe2hMu
Without a group length the parser reads the first element whatever its group, then walks group 0002; inflatedSize's walk stopped at a first element of another group and gave a deflated input no inflate allowance. Found by FuzzParse; a generated seed reproduces it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxyUkC8uMy3gASkRhe2hMu
…fallback - SpecificCharacterSet with a non-string value takes the default character set in every mode again, as before. - Only standard transfer syntaxes whose dataset is plain explicit VR little endian fall back to it; JPIP Referenced Deflate and JPIP HTJ2K Referenced Deflate are refused, since the parser inflates only Deflated Explicit VR Little Endian. - readNativeFrames reads, decodes and appends one frame at a time in every mode, reusing one frame buffer (8-bit frames keep their bytes), so frames reach the frame channel as read and nothing is allocated ahead of the bytes. - The tiny-frame check bounds the real cost (frame objects, unpacked samples, frame buffer) against nativeAllocMultiple: 10x for 8/16/32 bits, 73x for 1, so small 1-bit segmentations parse while one-byte frames stay refused. - Integer and float values start with capacity by their VR's value size, and their PushLimit errors name the element and VR again. - AppendBytes/ReadBytes wrap their refusal of ErrorInsufficientBytesLeft; readRawItem relies on it instead of a second Available. - FuzzParse reads the group length as readHeader does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013NwpH6q5fAZSmMfiyB84MC
…list - A negative (0002,0000) or PushLimit could land the limit on LimitReadUntilEOF, so a 158-byte file allocated a meta element's whole declared length; both are refused. - Zero frames no longer count a frame buffer against the size check, and Frames stays an empty list rather than nil. - nativeFrameOverhead is 200: append growing Frames costs about 45 bytes a slot, not 8. - AppendBytes' and readNativeFrames' docs give the total grown, not just the final buffer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013NwpH6q5fAZSmMfiyB84MC
- SpecificCharacterSet held as UN or OB bytes is read as its CS string, so an anonymized file's real character set applies; bytes naming no known set take the default. - Under an exact limit, native PixelData longer than the bytes left is refused before any frame is decoded or sent; frames are allocated as read only without one, where 8-bit frames now read into the reused buffer and are cloned once each. - Native images are refused only past max(nativeAllocMultiple x vl, 16 MiB), so small multiframes with tiny frames parse. - Unknown uids under the standard transfer syntax root are explicit VR little endian again; JPIP Referenced Deflate and JPIP HTJ2K Referenced Deflate canonicalize to Deflated Explicit VR Little Endian, and the parser inflates whatever canonicalizes to it. ParseTransferSyntaxUID and Write behave as v1.1.0 for them. - CHANGES.md lists the behaviour changes since v1.1.0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BWbdKpD39rj8ZFLP4XmNVS
Each native PixelData element took its own 16 MiB allowance, so a dataset repeating a tiny-frame PixelData element allocated about 120 bytes per input byte. The allowance is now one budget per parse, and FuzzParse adds it to the per-byte bound instead of taking the larger of the two. The small-frame allocation cases are checked against their expected cost. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BWbdKpD39rj8ZFLP4XmNVS
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ENG-1549: bounds the parser's memory on malformed input. Memory now follows the bytes the input actually holds, and a malformed header value returns an error instead of panicking.
MaxSequenceDepthchanges the cap.1.2.840.10008.1.2.*transfer syntaxes are read as explicit VR little endian.CHANGES.mdlists every behaviour change from v1.1.0, including thepkg/dicomioAPI additions.This branch is tagged
v1.1.1-travv.1, and core already pins that tag (core MR !1096). Merging this PR only puts the fix on the fork'smain, so it's independent of the core merge. The branch is one commit behindmain(upstream suyashkumar#380, a benchmark-only change) and doesn't conflict with it.Testing: unit tests and fuzz targets;
FuzzParseran for 11 minutes, 7.69M execs, with no failures. The vendor-file tests run withDICOM_VENDOR_SEEDS. Five review rounds.🤖 Generated with Claude Code
https://claude.ai/code/session_01L7cWEoW4yuEdJo5PHQECps