From 434e35c490cf174cfdd48c6d8010b5ad5f401ac5 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 23 Feb 2022 14:00:06 -0600 Subject: [PATCH] Panic on resetting image slots back to anonymous memory (#3841) * Panic on resetting image slots back to anonymous memory This commit updates `Drop for MemoryImageSlot` to panic instead of ignoring errors when resetting memory back to a clean slate. On reading some of this code again for a different change I realized that if an error happens in `reset_with_anon_memory` it would be possible, depending on where another error happened, to leak memory from one image to another. For example if `clear_and_remain_ready` failed its `madvise` (for whatever reason) and didn't actually reset any memory, then if `Drop for MemoryImageSlot` also hit an error trying to remap memory (for whatever reason), then nothing about memory has changed and when the `MemoryImageSlot` is recreated it'll think that it's 0-length when actually it's a bit larger and may leak data. I don't think this is a serious problem since we don't know any situation under which the `madvise` would fail and/or the resetting with anonymous memory, but given that these aren't expected to fail I figure it's best to be a bit more defensive here and/or loud about failures. * Update a comment --- crates/runtime/src/cow.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/crates/runtime/src/cow.rs b/crates/runtime/src/cow.rs index 4a08ff662f..b81a184783 100644 --- a/crates/runtime/src/cow.rs +++ b/crates/runtime/src/cow.rs @@ -566,17 +566,18 @@ impl Drop for MemoryImageSlot { // over by the next MemoryImageSlot later. // // Since we're in drop(), we can't sanely return an error if - // this mmap fails. Let's ignore the failure if so; the next - // MemoryImageSlot to be created for this slot will try to overwrite - // the existing stale mappings, and return a failure properly - // if we still cannot map new memory. + // this mmap fails. Instead though the result is unwrapped here to + // trigger a panic if something goes wrong. Otherwise if this + // reset-the-mapping fails then on reuse it might be possible, depending + // on precisely where errors happened, that stale memory could get + // leaked through. // - // The exception to all of this is if the `unmap_on_drop` flag + // The exception to all of this is if the `clear_on_drop` flag // (which is set by default) is false. If so, the owner of // this MemoryImageSlot has indicated that it will clean up in some // other way. if self.clear_on_drop { - let _ = self.reset_with_anon_memory(); + self.reset_with_anon_memory().unwrap(); } } }