-
-
Notifications
You must be signed in to change notification settings - Fork 75
Automatic canary-based stack overflow detection #33
Conversation
I have a repo that I think I overflow pretty easily by changing window size from 16 to 64 . So wanted to try this. Oddly enough I get $ cargo run --release --example 4_11_stft_accelerometer_microfft
Finished release [optimized + debuginfo] target(s) in 0.06s
Running `probe-run --chip STM32F407VGTx target/thumbv7em-none-eabihf/release/examples/4_11_stft_accelerometer_microfft`
flashing program ..
DONE
resetting device
Error: RTT control block not found in target memory. Make sure RTT is initialized on the target. Note I actually get the same message in |
@jacobrosenthal Thanks for the report, we might need to put a delay in the RTT connection loop. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great! left some comments about the logic but nothing of concern
rtt = Some(entry.value() as u32); | ||
match name { | ||
"_SEGGER_RTT" => rtt = Some(entry.value() as u32), | ||
"__rust_alloc" | "__rg_alloc" | "__rdl_alloc" | "malloc" if !uses_heap => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't __rust_alloc
an implementation detail? for now it will make do though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it is
match region { | ||
MemoryRegion::Ram(ram) => { | ||
if let Some(old) = &ram_region { | ||
log::debug!("multiple RAM regions found ({:?} and {:?}), stack canary will not be available", old, ram); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the device may have two regions but the application may only use one; this logic will exclude that kind of applications
this is ok for now but should we file a ticket to tweak the logic further to allow those apps? (not sure what that logic would look like though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I've filed #39.
Fixes #31