Skip to content

Commit 2fdc2ed

Browse files
abrownabrown
authored
threads: add validation for shared calls (bytecodealliance#1736)
* threads: add validation for shared calls The shared-everything-threads [proposal] prevents `shared` contexts (e.g., in a `shared` function) from accessing unshared objects. This change adds validation to prevent `shared`-to-unshared calls. [proposal]: https://github.com/WebAssembly/shared-everything-threads * threads: refactor and add shared access checks This checks shareability of other kinds of shared objects (minus `shared` memories). * Replace `type_of_function` with `type_index_of_function` `WasmModuleResources` previously retrieved the entire `FuncType` but this obscured the sharedness of the function in question, since the shared flag is held a level up in `CompositeType`. By replacing `WasmModuleResources::type_of_function` with a more low-level `WasmModuleResources::type_index_of_function`, we can reuse the existing helper functions that retrieve a `FuncType` from a `CompositeType` and do the necessary sharedness checks. * Clean up `OperatorValidator::new_func`
1 parent 5b46a26 commit 2fdc2ed

File tree

9 files changed

+251
-96
lines changed

9 files changed

+251
-96
lines changed

‎crates/wasmparser/src/resources.rs‎

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -50,14 +50,11 @@ pub trait WasmModuleResources {
5050
/// The sub type must be canonicalized.
5151
fn sub_type_at(&self, type_index: u32) -> Option<&SubType>;
5252

53-
/// Returns the type id associated with the given function
54-
/// index.
53+
/// Returns the type ID associated with the given function index.
5554
fn type_id_of_function(&self, func_idx: u32) -> Option<CoreTypeId>;
5655

57-
/// Returns the `FuncType` associated with the given function index.
58-
///
59-
/// The function type must be canonicalized.
60-
fn type_of_function(&self, func_idx: u32) -> Option<&FuncType>;
56+
/// Returns the type index associated with the given function index.
57+
fn type_index_of_function(&self, func_index: u32) -> Option<u32>;
6158

6259
/// Returns the element type at the given index.
6360
///
@@ -153,8 +150,8 @@ where
153150
fn type_id_of_function(&self, func_idx: u32) -> Option<CoreTypeId> {
154151
T::type_id_of_function(self, func_idx)
155152
}
156-
fn type_of_function(&self, func_idx: u32) -> Option<&FuncType> {
157-
T::type_of_function(self, func_idx)
153+
fn type_index_of_function(&self, func_idx: u32) -> Option<u32> {
154+
T::type_index_of_function(self, func_idx)
158155
}
159156
fn check_heap_type(&self, t: &mut HeapType, offset: usize) -> Result<(), BinaryReaderError> {
160157
T::check_heap_type(self, t, offset)
@@ -210,8 +207,8 @@ where
210207
T::type_id_of_function(self, func_idx)
211208
}
212209

213-
fn type_of_function(&self, func_idx: u32) -> Option<&FuncType> {
214-
T::type_of_function(self, func_idx)
210+
fn type_index_of_function(&self, func_idx: u32) -> Option<u32> {
211+
T::type_index_of_function(self, func_idx)
215212
}
216213

217214
fn check_heap_type(&self, t: &mut HeapType, offset: usize) -> Result<(), BinaryReaderError> {

‎crates/wasmparser/src/validator/core.rs‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1331,9 +1331,8 @@ impl WasmModuleResources for OperatorValidatorResources<'_> {
13311331
self.module.types.get(*type_index as usize).copied()
13321332
}
13331333

1334-
fn type_of_function(&self, at: u32) -> Option<&FuncType> {
1335-
let type_index = self.module.functions.get(at as usize)?;
1336-
Some(self.sub_type_at(*type_index)?.composite_type.unwrap_func())
1334+
fn type_index_of_function(&self, at: u32) -> Option<u32> {
1335+
self.module.functions.get(at as usize).copied()
13371336
}
13381337

13391338
fn check_heap_type(&self, t: &mut HeapType, offset: usize) -> Result<()> {
@@ -1407,9 +1406,8 @@ impl WasmModuleResources for ValidatorResources {
14071406
self.0.types.get(type_index as usize).copied()
14081407
}
14091408

1410-
fn type_of_function(&self, at: u32) -> Option<&FuncType> {
1411-
let type_index = *self.0.functions.get(at as usize)?;
1412-
Some(self.sub_type_at(type_index)?.composite_type.unwrap_func())
1409+
fn type_index_of_function(&self, at: u32) -> Option<u32> {
1410+
self.0.functions.get(at as usize).copied()
14131411
}
14141412

14151413
fn check_heap_type(&self, t: &mut HeapType, offset: usize) -> Result<()> {

‎crates/wasmparser/src/validator/func.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -276,7 +276,7 @@ mod tests {
276276
fn type_id_of_function(&self, _at: u32) -> Option<CoreTypeId> {
277277
todo!()
278278
}
279-
fn type_of_function(&self, _func_idx: u32) -> Option<&crate::FuncType> {
279+
fn type_index_of_function(&self, _at: u32) -> Option<u32> {
280280
todo!()
281281
}
282282
fn check_heap_type(&self, _t: &mut HeapType, _offset: usize) -> Result<()> {

‎crates/wasmparser/src/validator/operators.rs‎

Lines changed: 65 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,9 @@ pub(crate) struct OperatorValidator {
5454
/// Offset of the `end` instruction which emptied the `control` stack, which
5555
/// must be the end of the function.
5656
end_which_emptied_control: Option<usize>,
57+
58+
/// Whether validation is happening in a shared context.
59+
shared: bool,
5760
}
5861

5962
// No science was performed in the creation of this number, feel free to change
@@ -274,6 +277,7 @@ impl OperatorValidator {
274277
operands,
275278
control,
276279
end_which_emptied_control: None,
280+
shared: false,
277281
}
278282
}
279283

@@ -300,17 +304,30 @@ impl OperatorValidator {
300304
unreachable: false,
301305
init_height: 0,
302306
});
303-
let params = OperatorValidatorTemp {
304-
// This offset is used by the `func_type_at` and `inputs`.
307+
308+
// Retrieve the function's type via index (`ty`); the `offset` is
309+
// necessary due to `sub_type_at`'s error messaging.
310+
let sub_ty = OperatorValidatorTemp {
305311
offset,
306312
inner: &mut ret,
307313
resources,
308314
}
309-
.func_type_at(ty)?
310-
.params();
311-
for ty in params {
312-
ret.locals.define(1, *ty);
313-
ret.local_inits.push(true);
315+
.sub_type_at(ty)?;
316+
317+
// Set up the function's locals.
318+
if let CompositeInnerType::Func(func_ty) = &sub_ty.composite_type.inner {
319+
for ty in func_ty.params() {
320+
ret.locals.define(1, *ty);
321+
ret.local_inits.push(true);
322+
}
323+
} else {
324+
bail!(offset, "expected func type at index {ty}, found {sub_ty}")
325+
}
326+
327+
// If we're in a shared function, ensure we do not access unshared
328+
// objects.
329+
if sub_ty.composite_type.shared {
330+
ret.shared = true;
314331
}
315332
Ok(ret)
316333
}
@@ -912,14 +929,13 @@ where
912929
/// Returns the corresponding function type for the `func` item located at
913930
/// `function_index`.
914931
fn type_of_function(&self, function_index: u32) -> Result<&'resources FuncType> {
915-
match self.resources.type_of_function(function_index) {
916-
Some(f) => Ok(f),
917-
None => {
918-
bail!(
919-
self.offset,
920-
"unknown function {function_index}: function index out of bounds",
921-
)
922-
}
932+
if let Some(type_index) = self.resources.type_index_of_function(function_index) {
933+
self.func_type_at(type_index)
934+
} else {
935+
bail!(
936+
self.offset,
937+
"unknown function {function_index}: function index out of bounds",
938+
)
923939
}
924940
}
925941

@@ -971,7 +987,7 @@ where
971987
/// table.
972988
///
973989
/// The return value of this function is the function type behind
974-
/// `type_index` which must then be passedt o `check_{call,return_call}_ty`.
990+
/// `type_index` which must then be passed to `check_{call,return_call}_ty`.
975991
fn check_call_indirect_ty(
976992
&mut self,
977993
type_index: u32,
@@ -1302,6 +1318,12 @@ where
13021318
fn struct_type_at(&self, at: u32) -> Result<&'resources StructType> {
13031319
let sub_ty = self.sub_type_at(at)?;
13041320
if let CompositeInnerType::Struct(struct_ty) = &sub_ty.composite_type.inner {
1321+
if self.inner.shared && !sub_ty.composite_type.shared {
1322+
bail!(
1323+
self.offset,
1324+
"shared functions cannot access unshared structs",
1325+
);
1326+
}
13051327
Ok(struct_ty)
13061328
} else {
13071329
bail!(
@@ -1342,6 +1364,12 @@ where
13421364
fn array_type_at(&self, at: u32) -> Result<FieldType> {
13431365
let sub_ty = self.sub_type_at(at)?;
13441366
if let CompositeInnerType::Array(array_ty) = &sub_ty.composite_type.inner {
1367+
if self.inner.shared && !sub_ty.composite_type.shared {
1368+
bail!(
1369+
self.offset,
1370+
"shared functions cannot access unshared arrays",
1371+
);
1372+
}
13451373
Ok(array_ty.0)
13461374
} else {
13471375
bail!(
@@ -1365,6 +1393,12 @@ where
13651393
fn func_type_at(&self, at: u32) -> Result<&'resources FuncType> {
13661394
let sub_ty = self.sub_type_at(at)?;
13671395
if let CompositeInnerType::Func(func_ty) = &sub_ty.composite_type.inner {
1396+
if self.inner.shared && !sub_ty.composite_type.shared {
1397+
bail!(
1398+
self.offset,
1399+
"shared functions cannot access unshared functions",
1400+
);
1401+
}
13681402
Ok(func_ty)
13691403
} else {
13701404
bail!(
@@ -1382,6 +1416,12 @@ where
13821416

13831417
fn global_type_at(&self, at: u32) -> Result<GlobalType> {
13841418
if let Some(ty) = self.resources.global_at(at) {
1419+
if self.inner.shared && !ty.shared {
1420+
bail!(
1421+
self.offset,
1422+
"shared functions cannot access unshared globals",
1423+
);
1424+
}
13851425
Ok(ty)
13861426
} else {
13871427
bail!(self.offset, "unknown global: global index out of bounds");
@@ -1391,7 +1431,15 @@ where
13911431
/// Validates that the `table` is valid and returns the type it points to.
13921432
fn table_type_at(&self, table: u32) -> Result<TableType> {
13931433
match self.resources.table_at(table) {
1394-
Some(ty) => Ok(ty),
1434+
Some(ty) => {
1435+
if self.inner.shared && !ty.shared {
1436+
bail!(
1437+
self.offset,
1438+
"shared functions cannot access unshared tables",
1439+
);
1440+
}
1441+
Ok(ty)
1442+
}
13951443
None => bail!(
13961444
self.offset,
13971445
"unknown table {table}: table index out of bounds"

‎tests/local/shared-everything-threads/funcs.wast‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
;; Check that shared functions are valid WAT.
2+
(module
3+
(type $f (shared (func)))
4+
(func (import "spectest" "shared-func") (type $f))
5+
(func (type $f))
6+
)
7+
8+
;; Check that unshared functions cannot be called from shared functions.
9+
(assert_invalid
10+
(module
11+
(type $shared (shared (func)))
12+
(type $unshared (func))
13+
(func (type $shared)
14+
call $unshared)
15+
(func $unshared (type $unshared))
16+
)
17+
"shared functions cannot access unshared functions")
18+
19+
;; Check that unshared globals cannot be accessed from shared functions.
20+
(assert_invalid
21+
(module
22+
(global $unshared_global i32 (i32.const 0))
23+
(type $shared_func (shared (func)))
24+
(func (type $shared_func)
25+
global.get $unshared_global
26+
drop)
27+
)
28+
"shared functions cannot access unshared globals")
29+
30+
;; Check that unshared tables cannot be accessed from shared functions.
31+
(assert_invalid
32+
(module
33+
(table $unshared_table 1 anyref)
34+
(type $shared_func (shared (func)))
35+
(func (type $shared_func)
36+
i32.const 0
37+
table.get $unshared_table
38+
drop)
39+
)
40+
"shared functions cannot access unshared tables")
41+
42+
;; Check that unshared arrays cannot be accessed from shared functions.
43+
(assert_invalid
44+
(module
45+
(type $unshared_array (array anyref))
46+
(type $shared_func (shared (func)))
47+
(func (type $shared_func)
48+
(array.new $unshared_array (ref.null any) (i32.const 0))
49+
(array.get $unshared_array (i32.const 0))
50+
drop)
51+
)
52+
"shared functions cannot access unshared arrays")
53+
54+
;; Check that unshared structs cannot be accessed from shared functions.
55+
(assert_invalid
56+
(module
57+
(type $unshared_struct (struct (field i32)))
58+
(type $shared_func (shared (func)))
59+
(func (type $shared_func)
60+
(struct.new $unshared_struct (i32.const 0))
61+
(struct.get $unshared_struct 0)
62+
drop)
63+
)
64+
"shared functions cannot access unshared structs")

‎tests/local/shared-everything-threads/types.wast‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,3 @@
1-
;; Check shared attributes for global heap types.
2-
31
;; `func` references.
42
(module
53
;; Imported (long/short forms, mut, null).

‎tests/snapshots/local/shared-everything-threads/funcs.wast.json‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
{
2+
"source_filename": "tests/local/shared-everything-threads/funcs.wast",
3+
"commands": [
4+
{
5+
"type": "module",
6+
"line": 2,
7+
"filename": "funcs.0.wasm"
8+
},
9+
{
10+
"type": "assert_invalid",
11+
"line": 10,
12+
"filename": "funcs.1.wasm",
13+
"text": "shared functions cannot access unshared functions",
14+
"module_type": "binary"
15+
},
16+
{
17+
"type": "assert_invalid",
18+
"line": 21,
19+
"filename": "funcs.2.wasm",
20+
"text": "shared functions cannot access unshared globals",
21+
"module_type": "binary"
22+
},
23+
{
24+
"type": "assert_invalid",
25+
"line": 32,
26+
"filename": "funcs.3.wasm",
27+
"text": "shared functions cannot access unshared tables",
28+
"module_type": "binary"
29+
},
30+
{
31+
"type": "assert_invalid",
32+
"line": 44,
33+
"filename": "funcs.4.wasm",
34+
"text": "shared functions cannot access unshared arrays",
35+
"module_type": "binary"
36+
},
37+
{
38+
"type": "assert_invalid",
39+
"line": 56,
40+
"filename": "funcs.5.wasm",
41+
"text": "shared functions cannot access unshared structs",
42+
"module_type": "binary"
43+
}
44+
]
45+
}

‎tests/snapshots/local/shared-everything-threads/funcs.wast/0.print‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
(module
2+
(type $f (;0;) (shared(func)))
3+
(import "spectest" "shared-func" (func (;0;) (type $f)))
4+
(func (;1;) (type $f))
5+
)

0 commit comments

Comments
 (0)