Fix possibly harmful use of stale pointer in edge case.
The call to _MakeSpace() may move the extent data from the indirect array (kept in a heap allocation) to the direct one kept inside the class. In that case the lastExtent pointer would become stale and further use of it would've lead to suboptimal extents in the best case to reading/writing at the wrong point in files and possibly corruption of another allocation in the worst (both unlikely though). To mitigate that we now re-initialize the pointer to the correct location if we hit the cache limit. Also made the use of the start variable more understandable. Instaed of decrementing it (possibly wrapping) when an extent wasn't going to be used and later adding the vector index again, just increment whenever we actually move to the next extent. For bad things to happen a few conditions needed to come together though: 1. There needed to be multiple vectors that could be combined with the existing last extent. 2. There first needed to be more extents than the cache limit and that number then had to decrease below the cache limit again. 3. The memory needed to stay intact after being freed up until after the evaluation (or similar enough data had to be written to it). At least the last one was guaranteed to not be true anymore since we re-introduced overwritting freed memory with 0xdeadbeef in the slab, therefore nastily hiding this. I'm not sure that the first condition is ever met either (probably the vectors are combined beforehand so that there never are multiple adjacent ones) at least for the normal use case (the page writer writing back pages). I was at least unable to reproduce an actual file corruption in my testing. Just the out of bounds access to the stale pointer happened rather easily though and is now at least fixed.
This commit is contained in:
parent
e62d9911ea
commit
01762bd57f
11
src/system/kernel/cache/file_map.cpp
vendored
11
src/system/kernel/cache/file_map.cpp
vendored
@ -245,15 +245,22 @@ FileMap::_Add(file_io_vec* vecs, size_t vecCount, off_t& lastOffset)
|
||||
if (lastExtent->disk.offset + lastExtent->disk.length
|
||||
== vecs[i].offset
|
||||
|| (lastExtent->disk.offset == -1 && vecs[i].offset == -1)) {
|
||||
|
||||
lastExtent->disk.length += vecs[i].length;
|
||||
offset += vecs[i].length;
|
||||
start--;
|
||||
|
||||
_MakeSpace(fCount - 1);
|
||||
if (fCount == CACHED_FILE_EXTENTS) {
|
||||
// We moved the indirect array into the direct one, making
|
||||
// lastExtent a stale pointer, re-get it.
|
||||
lastExtent = ExtentAt(start - 1);
|
||||
}
|
||||
|
||||
continue;
|
||||
}
|
||||
}
|
||||
|
||||
file_extent* extent = ExtentAt(start + i);
|
||||
file_extent* extent = ExtentAt(start++);
|
||||
extent->offset = offset;
|
||||
extent->disk = vecs[i];
|
||||
|
||||
|
Loading…
Reference in New Issue
Block a user