Skip to content

Commit

Permalink
Call-indirect caching: protect against out-of-bounds table index duri…
Browse files Browse the repository at this point in the history
…ng prescan. (#8541)

* Call-indirect caching: protect against out-of-bounds table index during prescan.

Call-indirect caching requires a "prescan" of a module's code section
during compilation in order to know which tables are possibly written,
and to count call-indirect callsites so that each separate function
compilation can enable caching if possible and use the right slot
range.

This prescan is not integrated with the usual validation logic (nor
should it be, probably, for performance), so it's possible that an
out-of-bounds table index or other illegal instruction could be
present. We previously indexed into an internal data
structure (`table_plans`) with this index, allowing for a compilation
panic on certain invalid modules before validation would have caught
it. This PR fixes that with a one-off check at the single point that
we interpret a parameter (the table index) from an instruction during
the prescan.

* Add test case.
  • Loading branch information
cfallin committed May 3, 2024
1 parent 3f98505 commit 24c1388
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 1 deletion.
10 changes: 9 additions & 1 deletion crates/environ/src/compile/module_environ.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use crate::{
};
use anyhow::{bail, Result};
use cranelift_entity::packed_option::ReservedValue;
use cranelift_entity::EntityRef;
use std::borrow::Cow;
use std::collections::HashMap;
use std::mem;
Expand Down Expand Up @@ -737,7 +738,14 @@ and for re-adding support for interface types you can see this issue:
| Operator::TableCopy {
dst_table: table, ..
} => {
self.flag_written_table(TableIndex::from_u32(table));
// We haven't yet validated the body during
// this pre-scan, so we need to check that
// `dst_table` is in bounds. Ignore if not:
// we'll catch the error later.
let table = TableIndex::from_u32(table);
if table.index() < self.result.module.table_plans.len() {
self.flag_written_table(table);
}
}
// Count the `call_indirect` sites so we can
// assign them unique slots.
Expand Down
28 changes: 28 additions & 0 deletions tests/all/module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -269,3 +269,31 @@ fn call_indirect_caching_and_memory64() -> Result<()> {
)?;
Ok(())
}

#[test]
fn call_indirect_caching_out_of_bounds_table_index() -> Result<()> {
let mut config = Config::new();
config.cache_call_indirects(true);
let engine = Engine::new(&config)?;
// Test an out-of-bounds table index: this is exposed to the prescan
// that call-indirect caching must perform during compilation, so we
// need to make sure the error is properly handled by the validation
// that comes later.
let err = Module::new(
&engine,
"(module
(func (param i32)
ref.null func
local.get 0
table.set 32 ;; out-of-bounds table index
)
)",
)
.unwrap_err();
let err = format!("{err:?}");
assert!(
err.contains("table index out of bounds"),
"bad error: {err}"
);
Ok(())
}

0 comments on commit 24c1388

Please sign in to comment.