Skip to content

Commit bf1b863

Browse files
authored
Further limit memory growth during fuzzing (bytecodealliance#10026)
Previously a limit was added which prevented more than N growths over time but this wasn't sufficient to prevent a test case from continuously growing memory by just enough that it never went over N but the byte sizes in question were big enough that the fuzz test case timed out on OSS-Fuzz. This commit changes the check to limit "bytes moved" instead of the quantity of growths over time. This is a coarse approximation of what's happening but should hopefully still allow interesting behavior while additionally ensuring we don't spent the whole time moving around gigabytes of data.
1 parent c72de0b commit bf1b863

File tree

1 file changed

+31
-17
lines changed

1 file changed

+31
-17
lines changed

crates/fuzzing/src/oracles.rs

Lines changed: 31 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -70,43 +70,57 @@ pub struct StoreLimits(Arc<LimitsState>);
7070
struct LimitsState {
7171
/// Remaining memory, in bytes, left to allocate
7272
remaining_memory: AtomicUsize,
73-
/// Remaining times memories/tables can be grown
74-
remaining_growths: AtomicUsize,
73+
/// Remaining amount of memory that's allowed to be copied via a growth.
74+
remaining_copy_allowance: AtomicUsize,
7575
/// Whether or not an allocation request has been denied
7676
oom: AtomicBool,
7777
}
7878

79+
/// Allow up to 1G which is well below the 2G limit on OSS-Fuzz and should allow
80+
/// most interesting behavior.
81+
const MAX_MEMORY: usize = 1 << 30;
82+
83+
/// Allow up to 4G of bytes to be copied (conservatively) which should enable
84+
/// growth up to `MAX_MEMORY` or at least up to a relatively large amount.
85+
const MAX_MEMORY_MOVED: usize = 4 << 30;
86+
7987
impl StoreLimits {
8088
/// Creates the default set of limits for all fuzzing stores.
8189
pub fn new() -> StoreLimits {
8290
StoreLimits(Arc::new(LimitsState {
83-
// Limits tables/memories within a store to at most 1gb for now to
84-
// exercise some larger address but not overflow various limits.
85-
remaining_memory: AtomicUsize::new(1 << 30),
86-
// Also limit the number of times a memory or table may be grown.
87-
// Otherwise infinite growths can exhibit quadratic behavior. For
88-
// example Wasmtime could be configured with dynamic memories and no
89-
// guard regions to grow into, meaning each memory growth could be a
90-
// `memcpy`. As more data is added over time growths get more and
91-
// more expensive meaning that fuel may not be effective at limiting
92-
// execution time.
93-
remaining_growths: AtomicUsize::new(1000),
91+
remaining_memory: AtomicUsize::new(MAX_MEMORY),
92+
remaining_copy_allowance: AtomicUsize::new(MAX_MEMORY_MOVED),
9493
oom: AtomicBool::new(false),
9594
}))
9695
}
9796

9897
fn alloc(&mut self, amt: usize) -> bool {
9998
log::trace!("alloc {amt:#x} bytes");
99+
100+
// Assume that on each allocation of memory that all previous
101+
// allocations of memory are moved. This is pretty coarse but is used to
102+
// help prevent against fuzz test cases that just move tons of bytes
103+
// around continuously. This assumes that all previous memory was
104+
// allocated in a single linear memory and growing by `amt` will require
105+
// moving all the bytes to a new location. This isn't actually required
106+
// all the time nor does it accurately reflect what happens all the
107+
// time, but it's a coarse approximation that should be "good enough"
108+
// for allowing interesting fuzz behaviors to happen while not timing
109+
// out just copying bytes around.
110+
let prev_size = MAX_MEMORY - self.0.remaining_memory.load(SeqCst);
100111
if self
101112
.0
102-
.remaining_growths
103-
.fetch_update(SeqCst, SeqCst, |remaining| remaining.checked_sub(1))
113+
.remaining_copy_allowance
114+
.fetch_update(SeqCst, SeqCst, |remaining| remaining.checked_sub(prev_size))
104115
.is_err()
105116
{
106117
self.0.oom.store(true, SeqCst);
107-
log::debug!("too many growths, rejecting allocation");
118+
log::debug!("-> too many bytes moved, rejecting allocation");
108119
return false;
109120
}
121+
122+
// If we're allowed to move the bytes, then also check if we're allowed
123+
// to actually have this much residence at once.
110124
match self
111125
.0
112126
.remaining_memory
@@ -115,7 +129,7 @@ impl StoreLimits {
115129
Ok(_) => true,
116130
Err(_) => {
117131
self.0.oom.store(true, SeqCst);
118-
log::debug!("OOM hit");
132+
log::debug!("-> OOM hit");
119133
false
120134
}
121135
}

0 commit comments

Comments
 (0)