Heap Corruption from Deserialize only in --release

[Boscop]

Why am I getting heap corruption (only in release build) from a derived Deserialize impl?

And how can I prevent this from happening?

>	ntdll.dll!00007ffb48211cd0()	Unknown
 	ntdll.dll!00007ffb48214f12()	Unknown
 	ntdll.dll!00007ffb48215b10()	Unknown
 	ntdll.dll!00007ffb481ca61b()	Unknown
 	myproject.exe!serde::de::SeqAccess::next_element<bincode::de::{{impl}}::deserialize_tuple::Access<bincode::de::read::SliceReader, bincode::Infinite, byteorder::LittleEndian>,myproject::devices::launchpad::widget::NoteInputW>(bincode::de::{{impl}}::deserialize_tuple::Access<bincode::de::read::SliceReader, bincode::Infinite, byteorder::LittleEndian> * self) Line 1442	Unknown
 	myproject.exe!myproject::devices::launchpad::_IMPL_DESERIALIZE_FOR_SessionData::{{impl}}::deserialize::{{impl}}::visit_seq<bincode::de::{{impl}}::deserialize_tuple::Access<bincode::de::read::SliceReader, bincode::Infinite, byteorder::LittleEndian>>(myproject::devices::launchpad::_IMPL_DESERIALIZE_FOR_SessionData::{{impl}}::deserialize::__Visitor __seq, bincode::de::{{impl}}::deserialize_tuple::Access<bincode::de::read::SliceReader, bincode::Infinite, byteorder::LittleEndian>) Line 39	Unknown
 	myproject.exe!myproject::devices::launchpad::thread_fn_launchpad(alloc::arc::Arc<core::sync::atomic::AtomicBool> terminate, std::sync::mpsc::Receiver<window::MsgCoreToWin> rx_core_to_launchpad, std::sync::mpsc::Sender<myproject::devices::launchpad::MsgLaunchpadToCore> tx_launch_to_core2, std::sync::mpsc::Sender<myproject::devices::launchpad::MsgLaunchpadToDaw> tx_launch_to_daw, std::sync::mpsc::Receiver<myproject::devices::launchpad::MsgDawToLaunchpad> rx_daw_to_launchpad) Line 572	Unknown
 	myproject.exe!std::sys_common::backtrace::__rust_begin_short_backtrace<closure,()>(closure f) Line 136	Unknown
 	myproject.exe!std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()>(unsigned char * data) Line 308	Unknown
 	myproject.exe!panic_unwind::__rust_maybe_catch_panic() Line 102	Unknown
 	myproject.exe!alloc::boxed::{{impl}}::call_box<(),closure>(closure * self, ...) Line 783	Unknown
 	myproject.exe!std::sys::windows::thread::{{impl}}::new::thread_start() Line 55	Unknown
 	[External Code]	

[jethrogb]

Heap corruption is notoriously hard to track down, because the corruption might happen in one place and you won't notice it until you try to access that part the heap again, possibly in an entirely different function.

Do you you use any FFI or other unsafe code? If so, are you sure you are upholding all the invariants?

[kornel]

Run the code in valgrind.

Review all your unsafe blocks. They're or course the prime suspect.

[Boscop]

@jethrogb Yes, I'm certain I'm upholding all the invariants and that it's caused by Deserialize (see below..)

@kornel Thanks! I'm on Windows though, where valgrind doesn't work, right?

I'm not using any unsafe code (just checked) and I DO think that this is caused by the Deserialize code, I was getting stack overflows from it before (in debug build) (had to increase the thread stack sizes) because some of my structs are quite large.
Btw, what I'm doing is: I serialize my app state to disk using the rustbreak crate, and on startup I deserialize from it.
And I'm not getting any heap corruption when I remove the disk cache file (which will cause the app to use the Default for all app state structs) and then run it (in release mode, too). So the heap corruption only happens when it actually executes the deserialize code for my app state structs!
So I'm very certain that it's caused by this, but why? The disk cache file is only 1.35 MB though, so the total size of all my app state structs that I'm deserializing is less than that.

Any idea what I can do to prevent this? Or why it's not happening in debug builds?

[jethrogb]

It's possible there's a bug in bincode. Have you tried using a different serialization format?

Why do you think you're seeing heap corruption in particular and not some other memory safety problem?

[Boscop]

I've just tried with yaml (as rustbreak backend) now, but I'm still getting heap corruption when loading the persistence file.

But this time it shows a different line:

 	ntdll.dll!00007ffb48211cd0()	Unknown
 	ntdll.dll!00007ffb48214f12()	Unknown
 	ntdll.dll!00007ffb48215b10()	Unknown
 	ntdll.dll!00007ffb481ca5ff()	Unknown
>	myproject.exe!fiji::a64::deserialize<mut serde_yaml::de::Deserializer*,core::option::Option<(usize, myproject::music::Voices, u8)>>(serde_yaml::de::Deserializer * deserializer) Line 94	Unknown
 	myproject.exe!serde_yaml::de::Deserializer::visit<myproject::devices::launchpad::widget::_IMPL_DESERIALIZE_FOR_NoteInputW::{{impl}}::deserialize::__Visitor>(myproject::devices::launchpad::widget::_IMPL_DESERIALIZE_FOR_NoteInputW::{{impl}}::deserialize::__Visitor self) Line 183	Unknown
 	myproject.exe!serde_yaml::de::{{impl}}::deserialize_any<myproject::devices::launchpad::widget::_IMPL_DESERIALIZE_FOR_NoteInputW::{{impl}}::deserialize::__Visitor>(serde_yaml::de::Deserializer * self, myproject::devices::launchpad::widget::_IMPL_DESERIALIZE_FOR_NoteInputW::{{impl}}::deserialize::__Visitor) Line 508	Unknown
 	myproject.exe!serde_yaml::de::Deserializer::visit<myproject::devices::launchpad::_IMPL_DESERIALIZE_FOR_SessionData::{{impl}}::deserialize::__Visitor>(myproject::devices::launchpad::_IMPL_DESERIALIZE_FOR_SessionData::{{impl}}::deserialize::__Visitor self) Line 183	Unknown
 	myproject.exe!serde_yaml::de::{{impl}}::deserialize_any<myproject::devices::launchpad::_IMPL_DESERIALIZE_FOR_SessionData::{{impl}}::deserialize::__Visitor>(serde_yaml::de::Deserializer * self, myproject::devices::launchpad::_IMPL_DESERIALIZE_FOR_SessionData::{{impl}}::deserialize::__Visitor) Line 508	Unknown
 	myproject.exe!myproject::devices::launchpad::thread_fn_launchpad(alloc::arc::Arc<core::sync::atomic::AtomicBool> terminate, std::sync::mpsc::Receiver<window::MsgCoreToWin> rx_core_to_launchpad, std::sync::mpsc::Sender<myproject::devices::launchpad::MsgLaunchpadToCore> tx_launch_to_core2, std::sync::mpsc::Sender<myproject::devices::launchpad::MsgLaunchpadToDaw> tx_launch_to_daw, std::sync::mpsc::Receiver<myproject::devices::launchpad::MsgDawToLaunchpad> rx_daw_to_launchpad) Line 572	Unknown
 	myproject.exe!std::sys_common::backtrace::__rust_begin_short_backtrace<closure,()>(closure f) Line 136	Unknown
 	myproject.exe!std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()>(unsigned char * data) Line 308	Unknown
 	myproject.exe!panic_unwind::__rust_maybe_catch_panic() Line 102	Unknown
 	myproject.exe!alloc::boxed::{{impl}}::call_box<(),closure>(closure * self, ...) Line 783	Unknown
 	myproject.exe!std::sys::windows::thread::{{impl}}::new::thread_start() Line 55	Unknown
 	[External Code]	

It points to the line that says result.clone_from_slice(&slice); here:

#[macro_export]
macro_rules! serde_array { ($m:ident, $n:expr) => {
	pub mod $m {
		use serde::{Deserialize, Deserializer};
		pub use $crate::serialize_array as serialize;
		use super::*;

		pub fn deserialize<'de, D, T>(deserializer: D) -> Result<[T; $n], D::Error>
		where D: Deserializer<'de>, T: Deserialize<'de> + 'de + /*Copy*/ Clone {
			let mut result: [T; $n] = unsafe { ::std::mem::uninitialized() };
			let slice: Vec<T> = Deserialize::deserialize(deserializer)?;
			if slice.len() != $n {
				return Err(::serde::de::Error::custom("input slice has wrong length"));
			}
			// result.copy_from_slice(&slice);
			result.clone_from_slice(&slice);
			Ok(result)
		}
	}
}}

I'm using this function to serialize struct members that are large static arrays like so:

serde_array!(a64, 64);
serde_array!(a120, 120);
serde_array!(a128, 128);

#[derive(Clone, Serialize, Deserialize)]
struct Foo {
	#[serde(with = "a64")]
	bar_data: [BarData; 64],
}

But it works fine in debug builds, could this really be the cause for the heap corruption?
If so, what would be the correct way to do this?

[jethrogb]

Are you sure it's heap corruption? Large static arrays are often the cause of stack overflows. I'm not sure what the current status is of Rust's stack overflow checking, but it's entirely possible that it doesn't catch all cases with 100% certainty. cc @alexcrichton. Although I'm not sure why you'd have bigger stacks in release builds compared to debug builds...

[Boscop]

I'm also surprised that it's saying it's heap corruption, because these same large structs were causing me stack overflows with opt-level=0 during deserialization before I increased the stack_size to 4MB (and I'm only using opt-level=1 for debug now anyway, to get acceptable real-time performance)..

Are you suggesting it could be wrongly reporting this as heap corruption when it really is a stack overflow again?

Btw, how is it detecting heap corruption / disambiguating from stack overflows?

[jethrogb]

Have you tried increasing the stack size even more?

[Boscop]

I just tried with 20 MB stack_size (still yaml backend), and I get the same heap corruption crash pointing to the same line

[jethrogb]

Did you write serde_array as quoted above? It does contain an unsafe block and I don't think it's correct. You are not initializing the memory. Then you are either returning an error, which will call drop on all the (uninitialized) elements, or you are using copy_from_slice to overwrite the slice, which will drop all the (uninitialized) elements. You should be allocating the array after you check for errors and then be using std::ptr::write to avoid calling the destructor.

[Boscop]

Ah right... So like this?

#[macro_export]
macro_rules! serde_array { ($m:ident, $n:expr) => {
	pub mod $m {
		use std::{ptr, mem};
		use serde::{Deserialize, Deserializer};
		pub use $crate::serialize_array as serialize;
		use super::*;

		pub fn deserialize<'de, D, T>(deserializer: D) -> Result<[T; $n], D::Error>
		where D: Deserializer<'de>, T: Deserialize<'de> + 'de + Clone {
			let slice: Vec<T> = Deserialize::deserialize(deserializer)?;
			if slice.len() != $n {
				return Err(::serde::de::Error::custom("input slice has wrong length"));
			}
			unsafe {
				let mut result: [T; $n] = mem::uninitialized();
				for (src, dst) in slice.into_iter().zip(&mut result[..]) {
					ptr::write(dst, src);
				}
				Ok(result)
			}
		}
	}
}}

[jethrogb]

That looks correct. You don't need the Clone bound anymore.

[Boscop]

That was it, it works now!
Thanks so much!

Btw, here is my final code now (incl. serializer), in case someone wants to use it too:

use serde::{Serialize, Serializer};

pub fn serialize_array<S, T>(array: &[T], serializer: S) -> Result<S::Ok, S::Error>
where S: Serializer, T: Serialize {
	array.serialize(serializer)
}

#[macro_export]
macro_rules! serde_array { ($m:ident, $n:expr) => {
	pub mod $m {
		use std::{ptr, mem};
		use serde::{Deserialize, Deserializer, de};
		pub use $crate::serialize_array as serialize;
		use super::*;

		pub fn deserialize<'de, D, T>(deserializer: D) -> Result<[T; $n], D::Error>
		where D: Deserializer<'de>, T: Deserialize<'de> + 'de {
			let slice: Vec<T> = Deserialize::deserialize(deserializer)?;
			if slice.len() != $n {
				return Err(de::Error::custom("input slice has wrong length"));
			}
			unsafe {
				let mut result: [T; $n] = mem::uninitialized();
				for (src, dst) in slice.into_iter().zip(&mut result[..]) {
					ptr::write(dst, src);
				}
				Ok(result)
			}
		}
	}
}}

Btw, as a bonus, in case anyone is interested, I have several other macros that I use to deal with arrays (or serde), e.g.:

/// works like arr![x; 128] even when T is not Copy, e.g. arr![foo.clone(); 16]
#[macro_export]
macro_rules! arr {
	($item:expr; $n:expr) => (unsafe {
		let mut a: [_; $n] = ::std::mem::uninitialized();
		for x in &mut a[..] {
			::std::ptr::write(x, $item);
		}
		a
	})
}

/// similar but constructs array from closure, passing index. like init_with_indices() from the init_with crate (but that one only supports arrays up to len 32, this one supports unlimited len)
#[macro_export]
macro_rules! arr_from {
	($f:expr; $n:expr) => (unsafe {
		let mut a: [_; $n] = ::std::mem::uninitialized();
		for (i, x) in a.iter_mut().enumerate() {
			::std::ptr::write(x, $f(i));
		}
		a
	})
}

/// storing a type as a different type that T can be converted Into and From
macro_rules! serde_as {
	($m:ident, $t:ty, $ser:ty) => {
		pub mod $m {
			use serde::{Serialize, Serializer, Deserialize, Deserializer};
			use super::*;

			pub fn serialize<S: Serializer>(x: &$t, serializer: S) -> Result<S::Ok, S::Error> {
				let y: $ser = (*x).into();
				y.serialize(serializer)
			}

			pub fn deserialize<'de, D: Deserializer<'de>>(deserializer: D) -> Result<$t, D::Error> {
				let y: $ser = Deserialize::deserialize(deserializer)?;
				Ok(y.into())
			}
		}
	}
}

serde_as!(serde_col, RGB<f32>, (f32, f32, f32)); // RGB type from rgb crate

#[shrinkwrap(mutable)]
#[derive(Copy, Clone, PartialEq, Shrinkwrap, Serialize, Deserialize)]
pub struct Col(#[serde(with = "serde_col")] pub RGB<f32>);

[kornel]

Hey, so it was unsafe code!

BTW, don't wrangle with arrays. Use arrayvec. It handles initialization correctly, even if the code panics:

https://github.com/bluss/arrayvec/blob/16aabf7c2ca56f32e16e61ddb7e2b213eece9073/src/lib.rs#L855-L858

[Boscop]

@kornel Right, it was in unsafe code after all. (But not in my main project workspace of ~16000 lines, which doesn't contain unsafe..)

I think I tried ArrayVec before but it didn't fit my needs, I think it was because it's only impld for arrays of certain sizes, e.g. not for 120, which I need:

arrayvec::Array - Rust

And it doesn't expose the macro to generate an impl for a given N

Or can it be used for len 120 (and arbitrary other ones), too? (Before const generics land)

[vks]

@Boscop

And it doesn’t expose the macro to generate an impl for a given N

That wouldn't work because of Rust's orphan rules. You could however implement arrayvec::Array for struct MyArray([T; 120]), and arrayvec possibly could provide a macro for this.