Compression support

[ThalesMMS]

No description provided.

[claude]

Code Review: Compression Support PR

I've completed a thorough review of this PR adding compression support to DcmSwift. Here's my comprehensive feedback:

✅ Strengths

Architecture & Design

• Well-structured separation of concerns with CompressedPixelRouter centrally managing different compression formats
• Clean integration with existing PixelService without breaking changes
• Good use of Swift's type system and error handling

Code Quality

• Clear and consistent naming conventions throughout
• Proper use of guard statements and early returns
• Good modularization with separate decoders for each compression format

🐛 Potential Issues

1. Buffer Overflow Risk in FrameIndex.swift (Lines 103-106)

let nextOffset = (index + 1 < offsets.count) ? offsets[index + 1] : totalFragmentLength
let clampedNextOffset = min(max(nextOffset, startOffset), totalFragmentLength)
let length = clampedNextOffset - startOffset

The clamping logic is good, but consider adding validation for malformed BOT tables where offsets might be corrupted or deliberately crafted to cause issues.

2. Missing Validation in PixelSequence.basicOffsetTable()
The BOT parsing doesn't validate that offsets are monotonically increasing, which could lead to overlapping frames or negative lengths.

3. Error Recovery in DcmDecompress Tool
The batch testing continues silently on failures. Consider adding a summary of failed files or an option to stop on first failure for CI/CD scenarios.

⚡ Performance Considerations

1. Memory Usage: The frameCodestream() method in PixelSequence.swift concatenates all fragments into memory. For large multi-frame datasets, this could cause memory pressure. Consider streaming or chunked processing for large files.

2. Redundant Allocations: In CompressedPixelRouter, multiple data conversions occur (Data → [UInt8] → [UInt16]). Consider optimizing the pipeline to reduce intermediate allocations.

3. Missing Caching: Frame decoding results aren't cached. For applications that repeatedly access the same frames, consider adding an LRU cache.

🔒 Security Concerns

1. Input Validation: Add bounds checking for BOT offset values to prevent potential integer overflow attacks
2. Resource Limits: No apparent limits on decompression buffer sizes - consider adding configurable maximums
3. Untrusted Data: The code processes DICOM files which may come from untrusted sources. Consider adding:
   • Maximum frame size limits
   • Timeout mechanisms for decompression operations
   • Validation of compression parameters

🧪 Test Coverage

Good Coverage:

• Transfer syntax detection tests are comprehensive
• Frame index tests cover single/multi-frame scenarios
• Compression format tests validate basic functionality

Missing Coverage:

• Edge cases for corrupted BOT tables
• Performance regression tests for large files
• Error recovery scenarios
• Concurrent decompression stress tests
• Tests for malformed compressed data

📝 Recommendations

1. Add defensive checks in BOT parsing:

// Validate offsets are monotonically increasing
for i in 1..<offsets.count {
    guard offsets[i] > offsets[i-1] else {
        throw FrameIndexError.invalidBOTOffsets
    }
}

2. Add resource limits:

private let maxFrameSize = 512 * 1024 * 1024 // 512MB limit
guard length <= maxFrameSize else {
    throw FrameIndexError.frameTooLarge
}

3. Improve error messages in DcmDecompress for better debugging

4. Consider adding performance metrics collection for monitoring decompression performance in production

🎯 Overall Assessment

This is a solid implementation of DICOM compression support with good architectural decisions. The main areas for improvement are around defensive programming for handling potentially malicious input and optimizing memory usage for large datasets. With the suggested security hardening and additional test coverage, this will be a robust addition to the library.

The PR successfully adds valuable compression support while maintaining backward compatibility. Great work! 👍

[ThalesMMS]

@codex fix comments

[chatgpt-codex-connector]

Summary

• Added a maximum frame size cap, strict Basic Offset Table validation, and new frame index error cases to harden encapsulated and native frame indexing against malformed input. Sources/DcmSwift/Data/FrameIndex.swiftL34-L260
• Expanded the frame index test suite with scenarios covering invalid BOT offsets and oversized frames to verify the new defensive behavior. Tests/DcmSwiftTests/FrameIndexTests.swiftL158-L291
• Made the performance monitor fall back to a no-op implementation when os.signpost is unavailable so the package still builds on non-Apple platforms. Sources/DcmSwift/Graphics/DcmSwiftPerformanceMonitor.swiftL8-L83

Testing

• ⚠️ swift test (fails on Linux because the CoreGraphics module is unavailable)

View task →