Two related problems found in a code review. They must be fixed together.
Problem 1: pread / pwrite return values are never checked
partition_storage.cpp line 68 and line 101:
pread(m_fd, buffer, file_slice.size, partition_offset);
ret += file_slice.size; // always says the full read worked
...
pwrite(m_fd, buffer, file_slice.size, partition_offset);
The code does not check if the call failed (-1) or read/wrote fewer bytes than asked.
errno is never read. storage_error is never set. So a disk error looks like success.
Why this is bad on the leecher side (worst case):
A failed pwrite is reported as success. After that, async_hash checks the piece.
But async_hash reads the blocks from the cache, not from the disk
(raw_disk_io.cpp:462). The cache still has the good data. So the hash check passes,
but the disk does not have the data. The result: the deployed disk is broken, and no
check can see it. For a disk imaging tool this is the most serious possible bug.
Why this is bad on the seeder side:
A failed pread sends garbage bytes to peers as piece data. Leechers get
hash_failed, ban the seeder, and the swarm can stall. Today there is no log and no
error, so this is very hard to debug.
Fix idea:
- Loop until all bytes are moved (handle short reads/writes and
EINTR).
- On error, set
storage_error (error.ec = error_code(errno, system_category()),
error.operation = file_read / file_write) and return.
- All callers in
raw_disk_io.cpp already take a storage_error&, so they will do
the right thing once it is set.
Problem 2: prefetch path can return success with an empty buffer
In async_read, the chunk prefetch branch (raw_disk_io.cpp:276-302):
if (chunk_ret > 0 && !error) {
// insert blocks into cache
// copy the requested block into buf
}
// no else — handler is always posted after this
The copy into buf only happens inside the if. But the handler is always called.
So if the read returns chunk_ret <= 0 and error is not set, the handler gets
"no error" plus a buffer that was never filled. That uninitialized memory is then sent
to a peer as piece data.
There is also a second case: the cache insert loop uses chunk_size_bytes (expected
size), not chunk_ret (real size). After a short read, the missing tail is garbage,
but it is inserted into the cache as good data. Every later cache hit returns the same
garbage.
Fix idea: if chunk_ret <= 0 or the read is short, fall back to the single-block
read path, or make sure error is set before posting the handler back.
Why they must be fixed together
Today Problem 2 cannot happen: because of Problem 1, partition_storage::read always
returns the full size, so chunk_ret is never small. Problem 1 hides Problem 2.
If someone fixes Problem 1 alone, Problem 2 becomes live. So fix both in one change,
or fix Problem 1 first and Problem 2 right after.
Two related problems found in a code review. They must be fixed together.
Problem 1:
pread/pwritereturn values are never checkedpartition_storage.cppline 68 and line 101:The code does not check if the call failed (
-1) or read/wrote fewer bytes than asked.errnois never read.storage_erroris never set. So a disk error looks like success.Why this is bad on the leecher side (worst case):
A failed
pwriteis reported as success. After that,async_hashchecks the piece.But
async_hashreads the blocks from the cache, not from the disk(
raw_disk_io.cpp:462). The cache still has the good data. So the hash check passes,but the disk does not have the data. The result: the deployed disk is broken, and no
check can see it. For a disk imaging tool this is the most serious possible bug.
Why this is bad on the seeder side:
A failed
preadsends garbage bytes to peers as piece data. Leechers gethash_failed, ban the seeder, and the swarm can stall. Today there is no log and noerror, so this is very hard to debug.
Fix idea:
EINTR).storage_error(error.ec = error_code(errno, system_category()),error.operation = file_read / file_write) and return.raw_disk_io.cppalready take astorage_error&, so they will dothe right thing once it is set.
Problem 2: prefetch path can return success with an empty buffer
In
async_read, the chunk prefetch branch (raw_disk_io.cpp:276-302):The copy into
bufonly happens inside theif. But the handler is always called.So if the read returns
chunk_ret <= 0anderroris not set, the handler gets"no error" plus a buffer that was never filled. That uninitialized memory is then sent
to a peer as piece data.
There is also a second case: the cache insert loop uses
chunk_size_bytes(expectedsize), not
chunk_ret(real size). After a short read, the missing tail is garbage,but it is inserted into the cache as good data. Every later cache hit returns the same
garbage.
Fix idea: if
chunk_ret <= 0or the read is short, fall back to the single-blockread path, or make sure
erroris set before posting the handler back.Why they must be fixed together
Today Problem 2 cannot happen: because of Problem 1,
partition_storage::readalwaysreturns the full size, so
chunk_retis never small. Problem 1 hides Problem 2.If someone fixes Problem 1 alone, Problem 2 becomes live. So fix both in one change,
or fix Problem 1 first and Problem 2 right after.