Feature/issue 25 - #98
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 current_size() does not account for the new CRC32 checksum bytes, causing add() to overfill blocks
Block::current_size() (src/storage/block.rs:40-42) computes data.len() + metadata_size(offsets.len()) but does not include the 4-byte CRC32 checksum that encode() now appends. This means add() at line 47-50 compares against block_size without accounting for the CRC32 overhead, allowing blocks to be filled 4 bytes beyond the intended block_size limit when encoded. For small block sizes (e.g., block_size = 128 used in tests), this is a ~3% overshoot. It also causes encode() at line 70 to under-allocate Vec capacity by 4 bytes, triggering an unnecessary reallocation.
(Refers to lines 40-42)
Prompt for agents
The `current_size()` method needs to account for the 4-byte CRC32 checksum that `encode()` now appends. This affects two call sites: (1) `add()` uses it to check if the block is full — without accounting for CRC32, blocks can be slightly overfilled relative to `block_size`. (2) `encode()` uses it for `Vec::with_capacity` — causing an unnecessary reallocation. The fix should add `U32_SIZE` (for CRC32) to `metadata_size()` or `current_size()`. Note that `metadata_size` is called with `self.offsets.len()` (current number of offsets), and in `add()` the check also adds `new_offset_size` separately, so changing `metadata_size` should be safe. Alternatively, add a constant for the CRC32 footer size.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| pub fn decode(data: &[u8]) -> Self { | ||
| pub fn decode(data: &[u8]) -> std::result::Result<Self, LsmError> { | ||
| if data.len() < U32_SIZE { |
There was a problem hiding this comment.
🔴 Insufficient minimum length check in Block::decode causes panic on short input
Block::decode at src/storage/block.rs:90 checks data.len() < U32_SIZE (4 bytes), but the minimum valid encoded block requires 2 * U32_SIZE (8 bytes): 4 for the num_elements field + 4 for the CRC32 checksum. If data is 4–7 bytes and the CRC32 happens to match, line 120 computes data_without_checksum.len() - U32_SIZE which underflows usize, causing a panic.
This is deterministically triggerable: the input [0, 0, 0, 0] always panics because CRC32 of empty data is 0, so the checksum verification passes, then 0 - 4 underflows. This defeats the purpose of the CRC32 integrity check being added—corrupted or truncated block data that happens to be exactly 4–7 bytes crashes the process instead of returning a clean error.
| if data.len() < U32_SIZE { | |
| if data.len() < 2 * U32_SIZE { |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
📝 Description
Implements data integrity validation via CRC32 checksum on every SSTable block. From this change on, any on-disk corruption (bit rot, truncated writes, hardware faults) is detected at read time, returning
LsmError::CorruptedDatabefore invalid data reaches the engine.Closes #25.
🎯 Type of Change
feat:)fix:)docs:)style:/refactor:)perf:)build:/chore:)test:)🔍 What Changed?
Cargo.tomlcrc32fast = "1.4"src/storage/block.rsencode()— computes CRC32 viacrc32fast::Hasherover the full serialized content and appends 4 bytes (Little Endian) at the end of the blockdecode()— signature changed fromfn decode(data: &[u8]) -> Selftofn decode(data: &[u8]) -> Result<Self>; reads the last 4 bytes as the stored checksum, recomputes and compares — returnsLsmError::CorruptedDataon mismatchTests added (
src/storage/block.rs)test_crc32_corruption_detected— single byte flip in payload returnsLsmError::CorruptedDatatest_crc32_valid_checksum— encode/decode round-trip passes cleanlytest_crc32_checksum_mismatch_single_bit_flip— error message contains"mismatch"test_crc32_truncated_file_detected— file truncated without the final 4 checksum bytes is rejected