Skip to content

Protection against OOM when reading corrupted bincode #858

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

Merged
merged 2 commits into from
Mar 25, 2025
Merged

Conversation

spirali
Copy link
Collaborator

@spirali spirali commented Mar 25, 2025

No description provided.

Copy link
Collaborator

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Do you think that we can write a test for this? :D

@spirali
Copy link
Collaborator Author

spirali commented Mar 25, 2025

It is actually quite difficult, it seems that we need to write a large number on position where size of collection is written.
I can craft manually the file, but it would need to be be updated whenever the journal format changes.

I have put on my roadmap migration to bincode 2.0, so I will think about it again when I will do the update, as journal format would also need an update.

@spirali spirali merged commit 71a8939 into main Mar 25, 2025
9 checks passed
@spirali spirali deleted the bincode-fix branch March 25, 2025 14:40
@Kobzol
Copy link
Collaborator

Kobzol commented Mar 25, 2025

I think that we could use a custom serialize impl that would write a bogus size and then just small data.

@Kobzol
Copy link
Collaborator

Kobzol commented Mar 25, 2025

Also, when we load 2 (or N) identical events in a row, we should probably report an error (or at least a warning), because it's highly unlikely that several identical events (including identical timestamps) will be stored in the journal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants