search

cberner / redb

An embedded key-value database in pure Rust
https://www.redb.org
Apache License 2.0
3.38k stars 157 forks source link

Panic on invalid key/value string #874

Closed ClementNerma closed 1 month ago

ClementNerma commented 1 month ago

Hi there!

I've encountered a problem while using redb: when reading e.g. a String value from a table, if it's not valid UTF-8 (because of e.g. db corruption), the program crashes instead of returning an error. This seems caused by the following:

image

Values (and keys) decoding is typed as infallible, meaning whenever an invalid value is encountered, redb panics. This is problematic, and makes me wonder if the API shouldn't instead allow this method to return a Result<Self::SelfType, DecodingError> or something like this?

I know this would be a breaking change but getting a panic is not a good thing in such scenario

cberner commented 1 month ago

Ya, I considered that, but decided it was unnecessary. If you need to check that the database is valid, you should call check_integrity() Sent from my phone

On Sun, Oct 6, 2024, 5:11 AM Clément Nerma @.***> wrote:

Hi there!

I've encountered a problem while using redb: when reading e.g. a String value from a table, if it's not valid UTF-8 (because of e.g. db corruption), the program crashes instead of returning an error. This seems caused by the following:

image.png (view on web) https://github.com/user-attachments/assets/4a35fa62-42ba-4424-995e-7a6fc82314cf

Values (and keys) decoding is typed as infallible, meaning whenever an invalid value is encountered, redb panics. This is problematic, and makes me wonder if the API shouldn't instead allow this method to return a Result<Self::SelfType, DecodingError> or something like this?

I know this would be a breaking change but getting a panic is not a good thing in such scenario

— Reply to this email directly, view it on GitHub https://github.com/cberner/redb/issues/874, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGNXQDNH66QGKMHG6MFXPDZ2ESILAVCNFSM6AAAAABPOIRXNSVHI2DSMVQWIX3LMV43ASLTON2WKOZSGU3DQNRSHA3TSMI . You are receiving this because you are subscribed to this thread.Message ID: @.***>

ClementNerma commented 1 month ago

Oh check_integrity also ensures that all tables' keys and values can be decoded according to the provided types? If so that indeed solves the problem :) Thanks!

cberner commented 1 month ago

Technically, it ensures that the xxh3 hash of the encoded keys and values are the same. So if you're worried about malicious inputs, you may want your own cryptographic checks on top. But yes, for ordinary corruption it should be sufficient

ClementNerma commented 1 month ago

Oh then it's not what I thought, I'm more worried about e.g. a structure I'm putting into the database changing, resulting in a panic when I read the value from the database.

cberner commented 1 month ago

Ah. Ya, you shouldn't need to worry much about that. redb guarantees that the value you decode is the same one that you encoded, even if your OS crashes, as long as the storage media respects these assumptions: https://github.com/cberner/redb/blob/master/docs/design.md#assumptions-about-underlying-media

The tl;dr being that you won't hit a panic like that, unless you corrupt the database file with an external program, or your hard disk fails.