Repository navigation
Conversation
…sing Reason for change: Every size in the shm ingest path was client-controlled and never bounded against the partition: haveData's IPC numFrames was never compared to the request's maxFrames, DataReaderV1::readMetadata walked numFrames records unconditionally, DataReaderV2::readData advanced by client-written *metadataSize and metadata.length(), and MetadataV1.extraDataSize could exceed its 32-byte array. Corrupt client input could drive an OOB read past the partition - crashing the session server (and every session in it) or silently ingesting adjacent-partition data. haveDataInternal now clamps numFrames to the request's maxFrames and passes the partition length through createDataReader. DataReaderV1 clamps numFrames to region capacity, validates each segment's offset+length against the region, and clamps extraDataSize; DataReaderV2 bounds every read advance against the region end. Unaligned reinterpret_cast reads were replaced with memcpy as part of the same change. Test Procedure: Feed oversized numFrames / corrupted metadataSize/length/offset; the reader fails cleanly instead of walking out of bounds. Existing unit tests pass (mock signature and test call sites updated).
|
Pull request title must follow the pattern: Pull request description must follow the Commit message format for RDK-E:
< JIRA TICKET >: < one line summary of change less than 65 characters > |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The unit-test target no longer compiles, and critical bounds-safety defects cause undefined behavior in tests and allow a 32-bit length-check bypass that can lead to out-of-bounds reads.
4 open issues
What changed in this PR
This PR hardens the server’s shared-memory ingest path by checking client-controlled frame counts, offsets, and lengths against partition bounds.
Changes:
- Pass partition lengths through the reader factory and clamp incoming frame counts.
- Add bounds checks to V1 and V2 readers, clamp V1 extra data, and replace unaligned reads with
memcpy. - Update reader test calls and pipeline mocks for the new interfaces.
| File | Description |
|---|---|
| tests/unittests/media/server/mocks/main/DataReaderFactoryMock.h | Adds the partition-length argument to the mock. |
| tests/unittests/media/server/main/mediaPipeline/HaveDataTest.cpp | Updates factory-call expectations. |
| tests/unittests/media/server/main/mediaPipeline/base/MediaPipelineTestBase.cpp | Allows partition-length queries in the fixture. |
| tests/unittests/media/server/main/dataReader/DataReaderV2Tests.cpp | Supplies the new reader end-bound argument. |
| tests/unittests/media/server/main/dataReader/DataReaderV1Tests.cpp | Supplies the new reader region bounds. |
| media/server/main/source/MediaPipelineServerInternal.cpp | Retrieves partition length and clamps frame counts. |
| media/server/main/source/DataReaderV2.cpp | Checks metadata and payload bounds before advancing. |
| media/server/main/source/DataReaderV1.cpp | Bounds metadata counts and copies records with memcpy. |
| media/server/main/source/DataReaderFactory.cpp | Computes and passes partition bounds to readers. |
| media/server/main/include/IDataReaderFactory.h | Extends the factory interface with partition length. |
| media/server/main/include/DataReaderV2.h | Adds the region-end constructor argument and state. |
| media/server/main/include/DataReaderV1.h | Validates payload ranges and clamps extra-data size. |
| media/server/main/include/DataReaderFactory.h | Updates the concrete factory declaration. |
Review effort: Balanced
Comment on lines
+34
to
+35
| std::uint32_t dataOffset, std::uint32_t regionLen, | ||
| std::uint32_t numFrames, bool isBufferFull) const override; |
| } | ||
| currentReadPosition += *metadataSize; | ||
| currentReadPosition += metadataSize; | ||
| if (kDataEnd - currentReadPosition < static_cast<ptrdiff_t>(metadata.length())) |
| std::unique_ptr<IMediaPipeline::MediaSegment> readData(const firebolt::rialto::MediaSourceType &sourceType) | ||
| { | ||
| m_sut = std::make_unique<DataReaderV2>(sourceType, m_shm, kMetaDataSize, kNumFrames, kIsBufferFull); | ||
| m_sut = std::make_unique<DataReaderV2>(sourceType, m_shm, kMetaDataSize, kNumFrames, kIsBufferFull, UINT64_MAX); |
Comment on lines
+81
to
+82
| if (metadata.offset < m_regionStart || metadata.offset > m_regionEnd || | ||
| metadata.length > m_regionEnd - metadata.offset) |
This branch has not been deployed
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.
Reason for change:
Every size in the shm ingest path was client-controlled and never bounded against the partition: haveData's IPC numFrames was never compared to the request's maxFrames, DataReaderV1::readMetadata walked numFrames records unconditionally, DataReaderV2::readData advanced by client-written *metadataSize and metadata.length(), and MetadataV1.extraDataSize could exceed its 32-byte array. Corrupt client input could drive an OOB read past the partition - crashing the session server (and every session in it) or silently ingesting adjacent-partition data.
haveDataInternal now clamps numFrames to the request's maxFrames and passes the partition length through createDataReader. DataReaderV1 clamps numFrames to region capacity, validates each segment's offset+length against the region, and clamps extraDataSize; DataReaderV2 bounds every read advance against the region end. Unaligned reinterpret_cast reads were replaced with memcpy as part of the same change.
Test Procedure:
Feed oversized numFrames / corrupted metadataSize/length/offset; the reader fails cleanly instead of walking out of bounds. Existing unit tests pass (mock signature and test call sites updated).