blocksActuallyRead rounded bytesRead up to the next whole block
unconditionally, intending to handle the legitimate case where the
very last block of the file is naturally shorter than BLOCK_SIZE
(cache_ is deliberately over-allocated for that). But it applied the
same rounding to any short read, including a genuine failure or a
dropped connection mid-file (this loader can sit on top of the whole
Remote ISO chain via CachingFileLoader/HTTPFileLoader when "Cache full
ISO in RAM" is enabled) - marking a block as fully cached when only a
few of its bytes were actually written. Since cache_ is malloc'd (not
zeroed), every later read of that block would serve uninitialized heap
memory as if it were real file data.
Only round up when the short read's end position exactly matches the
true end of the file.
SaveIntoCache checked `readBytes != 0` instead of comparing against the
full expected length, so any nonzero-but-short read from the backend
(e.g. a Remote ISO connection dropping mid-file) was treated as a
complete success: in the multi-block path this marked *all* requested
blocks (up to 16) as fully cached and wrote the uninitialized tail of
the read buffer to the on-disk cache file, and in both paths the
short/uninitialized data was also copied straight into the caller's
output buffer and counted in the return value - so a read failure was
reported (and permanently cached) as success. Only treat a block as
read once the backend actually delivered the full blockSize_ for it,
and stop before caching or returning anything for blocks it didn't.
Also fixes two latent bugs in the same functions, unreachable in the
current call graph (DiskCachingFileLoader is only ever driven by
CachingFileLoader, which always issues block-aligned reads) but wrong
if ever called otherwise:
- The multi-block loop reused the batch's initial `offset` (the
position within the *first* block) for every subsequent block
instead of resetting it to 0, which would both read from the wrong
place in `wholeRead` and mis-copy less than a full block for i > 0.
- ReadBlockData() applied `offset` to the destination pointer instead
of the file seek position, which would both read the wrong bytes
from disk and write up to `offset` bytes past the end of the
caller's buffer.
LoadCacheIndex's sanity check on persisted block indices used `>`
instead of `>=` against maxBlocks_ (blockIndexLookup_ only has
maxBlocks_ entries, valid indices 0..maxBlocks_-1), so a corrupted
cache file's index entry with block == maxBlocks_ exactly would pass
validation and then index one past the end of blockIndexLookup_.
SaveIntoCache() discarded backend_->ReadAt()'s return value entirely
and unconditionally marked the requested block(s) as cached. A short
or failed read from the backend (e.g. a Remote ISO connection dropping
mid-file, now that LocalFileLoader/RetryingFileLoader correctly report
failures as 0 rather than a huge count) would still get stored as a
"valid" cached block, permanently serving its uninitialized tail as if
it were real file data on every later read, with no retry.
Only insert a block once we've confirmed the backend actually
delivered the full BLOCK_SIZE for it.
ReadAt()'s contract is to return the number of bytes/units actually
read. On every platform branch, an OS-level read failure (ReadFile
returning FALSE, or pread/read returning -1) was fed straight into a
division by `bytes` without checking for it first:
- Windows explicitly returned (size_t)-1.
- Elsewhere, the signed -1 from pread/read was implicitly converted to
size_t (via the usual arithmetic conversions with the unsigned
`bytes`) before the division, producing a huge bogus count instead
of a small one.
Every caller in the caching chain (CachingFileLoader,
RamCachingFileLoader, RetryingFileLoader, ZipFileLoader's libzip
source callback) loops on "did we get at least what we asked for",
which a huge return value trivially satisfies - so a local I/O error
(removable media ejected, a content-URI permission problem mid-read,
etc.) would be reported as a fully successful read of whatever
uninitialized memory happened to be in the destination buffer.
A malicious or MITM'd server could send a Content-Range header matching
the requested range but a larger entity body, overflowing the caller's
fixed-size buffer via output.Take. Clamp the copied size to the requested
range.
Turns out these were needed after all. For some reason, on Windows and
Mac, <algorithm> gets auto-included by something else so I don't notice
when it's missing, and MSVC's include dependency tracker doesn't see it
either.
sleep_ms() should generally be avoided when possible. This can be used to try
to track down unnecessary sleeps by adding some logging.
This commit on its own doesn't actually add any logging.
* Rename LogType to Log
* Explicitly use the Log:: enum when logging. Allows for autocomplete when editing.
* Mac/ARM64 buildfix
* Do the same with the hle result log macros
* Rename the log names to mixed case while at it.
* iOS buildfix
* Qt buildfix attempt, ARM32 buildfix
Henrik Rydgård