Corruption bug: log::Reader does not retry short reads
- Dominant language
- C++
- Stars
- 39.4k
- Forks
- 8.2k
- PR merge metrics
- No merged PRs in 30d
Description
`log::Reader` uses the `SequentialFile::Read` method provided by some Env. It seems that the implementations used by LevelDB do at least retry reads on EINTR, but if an interrupt occurs after some but not all of the bytes were read, then the Read call may succeed but return fewer bytes than requested.
You can see that the below code assumes that a successful short read would only ever occur due to EOF:
https://github.com/google/leveldb/blob/ac691084fdc5546421a55b25e7653d450e5a25fb/db/log_reader.cc#L194-L204
An implementation of `SequentialFile::Read`:
https://github.com/google/leveldb/blob/ac691084fdc5546421a55b25e7653d450e5a25fb/util/env_posix.cc#L142-L157
This seems ***very bad***. I'm not sure what the chance is of an interrupt actually occurring during a 32 KiB read, but any such interrupt could easily lead to data loss. `log::Reader::ReadRecord`, which can fall victim to the above bug, is called every time a database is opened, to read the current MANIFEST file (and any .log files).
- If the bug occurs when opening a .log file, some recently-written data is lost (without even being reported as corruption).
- If an interrupt happens at just the wrong time when opening the current MANIFEST file, it might be reported as corruption (resulting in the database failing to open) if the first VersionEdit of the MANIFEST failed to be read. This is probably the best-case scenario, since attempting to reopen the database would probably show that no data was actually lost.
- If any VersionEdit after the first in the current MANIFEST fails to be read, it and any following ones are silently not read, losing who-knows-how-many recently-added table files, and possibly resulting in corruption being reported later when the VersionSet thinks some previously-deleted table files should still exist (because a VersionEdit indicating their deletion was not read).
Ideally, `SequentialFile::Read` should clearly document that its implementors are required to retry on EINTR or equivalent, and as existing implementations may have successful short reads for non-EOF reasons, `log::Reader` must read data until either a successful read of length 0 occurs (EOF), the full block is successfully read, or an error occurs.
Contributor guide
Assessment
This issue has not been assessed yet.