Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Certain truncated files can trigger Python MemoryError #1220

Closed
michaeltandy opened this issue Aug 20, 2024 · 1 comment · Fixed by #1225
Closed

Certain truncated files can trigger Python MemoryError #1220

michaeltandy opened this issue Aug 20, 2024 · 1 comment · Fixed by #1225
Assignees
Labels
bug Something isn't working

Comments

@michaeltandy
Copy link

Description

  • Version: MCAP libraries for Python 1.1.1 with mcap-ros2-support 0.5.3
  • Platform:

Steps To Reproduce

Load a truncated file, such as this one: minimal-mcap-to-trigger-memory-error.mcap.gz

Using for example this code:

from mcap_ros2.reader import read_ros2_messages
for msg in read_ros2_messages('minimal-mcap-to-trigger-memory-error.mcap'):
    print(msg.ros_msg)
    break

Observed behaviour

(venv) mtandy@mtandy-tr:/tmp/mcap-test$ python3 basictest.py 
Traceback (most recent call last):
  File "/tmp/mcap-test/basictest.py", line 4, in <module>
    for msg in read_ros2_messages('demo-memoryerror.mcap'):
  File "/tmp/venv/lib/python3.12/site-packages/mcap_ros2/reader.py", line 74, in read_ros2_messages
    for schema, channel, message, ros2_msg in reader.iter_decoded_messages(
  File "/tmp/venv/lib/python3.12/site-packages/mcap/reader.py", line 191, in iter_decoded_messages
    for schema, channel, message in message_iterator:
  File "/tmp/venv/lib/python3.12/site-packages/mcap/reader.py", line 278, in iter_messages
    summary = self.get_summary()
              ^^^^^^^^^^^^^^^^^^
  File "/tmp/venv/lib/python3.12/site-packages/mcap/reader.py", line 338, in get_summary
    footer = next(StreamReader(self._stream, skip_magic=True).records)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/venv/lib/python3.12/site-packages/mcap/stream_reader.py", line 120, in records
    record = self._read_record(opcode, length)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/venv/lib/python3.12/site-packages/mcap/stream_reader.py", line 205, in _read_record
    self._stream.read(length - 9)
  File "/tmp/venv/lib/python3.12/site-packages/mcap/data_stream.py", line 29, in read
    data = self._stream.read(length)
           ^^^^^^^^^^^^^^^^^^^^^^^^^
MemoryError

Expected Behavior
mcap.exceptions.EndOfFile or similar

@michaeltandy michaeltandy added the bug Something isn't working label Aug 20, 2024
Copy link

linear bot commented Aug 20, 2024

james-rms added a commit that referenced this issue Aug 28, 2024
### Changelog
- Added: the python MCAP library reader classes gained a new parameter,
`record_size_limit`, defaulting to 4GiB. This allows callers to limit
the size of records that their application will support. When a record
is encountered with a greater length, the reader will raise an
`InvalidRecordLength` exception. This limit can be removed by setting it
to `None`. This helps applications avoid the issue where a corrupt MCAP
can cause a `MemoryError`.

### Docs

See generated python docs in the mcap.dev preview.
### Description

<!-- Describe the problem, what has changed, and motivation behind those
changes. Pretend you are advocating for this change and the reader is
skeptical. -->

<!-- In addition to unit tests, describe any manual testing you did to
validate this change. -->

<table><tr><th>Before</th><th>After</th></tr><tr><td>

<!--before content goes here-->

</td><td>

<!--after content goes here-->

</td></tr></table>

<!-- If necessary, link relevant Linear or Github issues. Use `Fixes:
foxglove/repo#1234` to auto-close the Github issue or Fixes: FG-### for
Linear isses. -->
Fixes: #1220
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging a pull request may close this issue.

2 participants