Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: reduce consumption of virtual address space. #295

Merged
merged 5 commits into from
Jan 20, 2025
Merged

fix: reduce consumption of virtual address space. #295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f1fc21d
fix: change wasmtime's memory reservation from 8GB to 2GB.
plusvic Jan 17, 2025
08016d2
fix: set a maximum size for WASM main memory and tweak configuration …
plusvic Jan 17, 2025
c585d73
chore: reduce WASM memory reservation from 2GB to 256MB.
plusvic Jan 18, 2025
4969488
chore: reduce WASM memory reservation to 16MB.
plusvic Jan 18, 2025
1f7c59b
refactor: document changes
plusvic Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/src/scanner/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ impl<'r> Scanner<'r> {
// Create module's main memory.
let main_memory = wasmtime::Memory::new(
wasm_store.as_context_mut(),
MemoryType::new(mem_size, None),
MemoryType::new(mem_size, Some(mem_size)),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Since Memory::new can fail, it's good practice to handle the potential error. While unwrapping is acceptable in some cases, explicitly handling the error enhances code robustness. Consider using the ? operator or a match statement to handle the Result returned by Memory::new. What specific error handling would be appropriate if memory allocation fails? Should the scan be aborted, or is there a fallback mechanism?

MemoryType::new(mem_size, Some(mem_size))?

)
.unwrap();

Expand Down
22 changes: 22 additions & 0 deletions lib/src/wasm/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -721,6 +721,28 @@ lazy_static! {

config.cranelift_opt_level(wasmtime::OptLevel::SpeedAndSize);
config.epoch_interruption(true);

// 16MB should be enough for each WASM module. Each module needs a
// fixed amount of memory that is only a few KB long, plus a variable
// amount that depends on the number of rules and patterns (1 bit per
// rule and 1 bit per pattern). With 16MB there's enough space for
// millions of rules and patterns. By default, this is 4GB in 64-bits
// systems, which causes a reservation of 4GB of virtual address space
// (not physical RAM) per module (and therefore per Scanner). In some
// scenarios where virtual address space is limited (i.e: Docker
// instances) this is problematic. See:
// https://github.com/VirusTotal/yara-x/issues/292
Comment on lines +725 to +734
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

low

The comment explaining the rationale for the 16MB limit is very clear. However, per the Rust API Guidelines, comments should focus on why something is done, rather than what is done. The what should be clear from the code itself. Consider rephrasing the comment to emphasize the why even more. For example, instead of "By default, this is 4GB...causes a reservation of 4GB...", you could say "Limiting to 16MB mitigates excessive virtual address space consumption, particularly in environments like Docker, where address space is limited (see #292)."

// Limiting to 16MB mitigates excessive virtual address space consumption,
        // particularly in environments like Docker, where address space is limited (see #292).
        // Each module requires a fixed amount of memory (a few KB), plus a variable
        // amount based on the number of rules and patterns (1 bit per rule and pattern).
        // 16MB is sufficient for millions of rules and patterns.

config.memory_reservation(0x1000000);

// WASM memory won't grow, there's no need to allocate space for
// future grow.
config.memory_reservation_for_growth(0);

// As the memory can't grow, it won't move. By explicitly indicating
// this, modules can be compiled with static knowledge the base pointer
// of linear memory never changes to enable optimizations.
config.memory_may_move(false);

config
};
pub(crate) static ref ENGINE: Engine = Engine::new(&CONFIG).unwrap();
Expand Down
Loading