Skip to content

Commit

Permalink
python: fix incorrect read length for custom records (#1311)
Browse files Browse the repository at this point in the history
### Changelog
Python: fixed a crash when reading files that contain custom record
types.

### Docs

None

### Description

`length - 9` was incorrect because the length of the opcode+length
fields are not included in the record length.
  • Loading branch information
jtbandes authored Jan 15, 2025
1 parent b59be11 commit 6c9ce28
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 9 deletions.
2 changes: 1 addition & 1 deletion python/mcap/mcap/__init__.py
Original file line number Diff line number Diff line change
@@ -1 +1 @@
__version__ = "1.2.1"
__version__ = "1.2.2"
2 changes: 1 addition & 1 deletion python/mcap/mcap/stream_reader.py
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ def _read_record(self, opcode: int, length: int) -> Optional[McapRecord]:
return SummaryOffset.read(self._stream)

# Skip unknown record types
self._stream.read(length - 9)
self._stream.read(length)


__all__ = ["StreamReader"]
36 changes: 29 additions & 7 deletions python/mcap/tests/test_reader.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
"""tests for the McapReader implementations."""

# cspell:words getbuffer
import json
import os
Expand All @@ -24,6 +25,14 @@
)


class StrictBytesIO(BytesIO):
"""A subclass of BytesIO that throws for negative or unspecified-length reads."""

def read(self, size: Union[int, None] = -1):
assert size is not None and size > 0
return super().read(size)


@pytest.fixture
def pipe():
r, w = os.pipe()
Expand Down Expand Up @@ -263,21 +272,21 @@ def test_detect_invalid_initial_magic(tmpdir: Path):

def test_record_size_limit():
# create a simple small MCAP
write_stream = BytesIO()
write_stream = StrictBytesIO()
writer = Writer(write_stream)
writer.start("profile", "library")
writer.finish()

# default stream reader can read it
stream_reader = StreamReader(
BytesIO(write_stream.getbuffer()), record_size_limit=100
StrictBytesIO(write_stream.getbuffer()), record_size_limit=100
)
records = [r for r in stream_reader.records]
assert len(records) == 10

# can cause "large" records to raise an error by setting a low limit
stream_reader = StreamReader(
BytesIO(write_stream.getbuffer()), record_size_limit=10
StrictBytesIO(write_stream.getbuffer()), record_size_limit=10
)
with pytest.raises(
RecordLengthLimitExceeded,
Expand All @@ -287,15 +296,15 @@ def test_record_size_limit():

# default seeking reader can read it
seeking_reader = SeekingReader(
BytesIO(write_stream.getbuffer()), record_size_limit=100
StrictBytesIO(write_stream.getbuffer()), record_size_limit=100
)
seeking_reader.get_header()
seeking_reader.get_summary()
assert len([m for m in seeking_reader.iter_messages()]) == 0

# can cause "large" records to raise an error by setting a low limit
seeking_reader = SeekingReader(
BytesIO(write_stream.getbuffer()), record_size_limit=10
StrictBytesIO(write_stream.getbuffer()), record_size_limit=10
)
with pytest.raises(
RecordLengthLimitExceeded,
Expand All @@ -311,16 +320,29 @@ def test_record_size_limit():

# default non-seeking reader can read it
non_seeking_reader = NonSeekingReader(
BytesIO(write_stream.getbuffer()), record_size_limit=100
StrictBytesIO(write_stream.getbuffer()), record_size_limit=100
)
non_seeking_reader.get_header()

# can cause "large" records to raise an error by setting a low limit
non_seeking_reader = NonSeekingReader(
BytesIO(write_stream.getbuffer()), record_size_limit=10
StrictBytesIO(write_stream.getbuffer()), record_size_limit=10
)
with pytest.raises(
RecordLengthLimitExceeded,
match="HEADER record has length 22 that exceeds limit 10",
):
non_seeking_reader.get_header()


def test_custom_record():
write_stream = StrictBytesIO()
writer = Writer(write_stream)
writer.start("profile", "library")
write_stream.write(b"\x80\x00\x00\x00\x00\x00\x00\x00\x00")
writer.finish()
stream_reader = StreamReader(
StrictBytesIO(write_stream.getbuffer()), record_size_limit=100
)
records = [r for r in stream_reader.records]
assert len(records) == 10

0 comments on commit 6c9ce28

Please sign in to comment.