Skip to content

Commit 6351c2d

Browse files
jimblandyjimblandy
authored
[naga] Forbid cycles between global expressions and types. (#6800)
* [naga] Move type handle validation into its own function. * [naga] Forbid cycles between global expressions and types. Update `valid::handles` to traverse `Module::types` and `Module::global_expressions` in tandem, to ensure that the types and expressions are acyclic. Fixes #6793.
1 parent 0d69482 commit 6351c2d

File tree

1 file changed

+179
-39
lines changed

1 file changed

+179
-39
lines changed

‎naga/src/valid/handles.rs

Lines changed: 179 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -44,36 +44,89 @@ impl super::Validator {
4444
ref diagnostic_filter_leaf,
4545
} = module;
4646

47-
// NOTE: Types being first is important. All other forms of validation depend on this.
48-
for (this_handle, ty) in types.iter() {
49-
match ty.inner {
50-
crate::TypeInner::Scalar { .. }
51-
| crate::TypeInner::Vector { .. }
52-
| crate::TypeInner::Matrix { .. }
53-
| crate::TypeInner::ValuePointer { .. }
54-
| crate::TypeInner::Atomic { .. }
55-
| crate::TypeInner::Image { .. }
56-
| crate::TypeInner::Sampler { .. }
57-
| crate::TypeInner::AccelerationStructure
58-
| crate::TypeInner::RayQuery => (),
59-
crate::TypeInner::Pointer { base, space: _ } => {
60-
this_handle.check_dep(base)?;
61-
}
62-
crate::TypeInner::Array { base, .. }
63-
| crate::TypeInner::BindingArray { base, .. } => {
64-
this_handle.check_dep(base)?;
65-
}
66-
crate::TypeInner::Struct {
67-
ref members,
68-
span: _,
69-
} => {
70-
this_handle.check_dep_iter(members.iter().map(|m| m.ty))?;
47+
// Because types can refer to global expressions and vice versa, to
48+
// ensure the overall structure is free of cycles, we must traverse them
49+
// both in tandem.
50+
//
51+
// Try to visit all types and global expressions in an order such that
52+
// each item refers only to previously visited items. If we succeed,
53+
// that shows that there cannot be any cycles, since walking any edge
54+
// advances you towards the beginning of the visiting order.
55+
//
56+
// Validate all the handles in types and expressions as we traverse the
57+
// arenas.
58+
let mut global_exprs_iter = global_expressions.iter().peekable();
59+
for (th, t) in types.iter() {
60+
// Imagine the `for` loop and `global_exprs_iter` as two fingers
61+
// walking the type and global expression arenas. They don't visit
62+
// elements at the same rate: sometimes one processes a bunch of
63+
// elements while the other one stays still. But at each point, they
64+
// check that the two ranges of elements they've visited only refer
65+
// to other elements in those ranges.
66+
//
67+
// For brevity, we'll say 'handles behind `global_exprs_iter`' to
68+
// mean handles that have already been produced by
69+
// `global_exprs_iter`. Once `global_exprs_iter` returns `None`, all
70+
// global expression handles are 'behind' it.
71+
//
72+
// At this point:
73+
//
74+
// - All types visited by prior iterations (that is, before
75+
// `th`/`t`) refer only to expressions behind `global_exprs_iter`.
76+
//
77+
// On the first iteration, this is obviously true: there are no
78+
// prior iterations, and `global_exprs_iter` hasn't produced
79+
// anything yet. At the bottom of the loop, we'll claim that it's
80+
// true for `th`/`t` as well, so the condition remains true when
81+
// we advance to the next type.
82+
//
83+
// - All expressions behind `global_exprs_iter` refer only to
84+
// previously visited types.
85+
//
86+
// Again, trivially true at the start, and we'll show it's true
87+
// about each expression that `global_exprs_iter` produces.
88+
//
89+
// Once we also check that arena elements only refer to prior
90+
// elements in that arena, we can see that `th`/`t` does not
91+
// participate in a cycle: it only refers to previously visited
92+
// types and expressions behind `global_exprs_iter`, and none of
93+
// those refer to `th`/`t`, because they passed the same checks
94+
// before we reached `th`/`t`.
95+
if let Some(max_expr) = Self::validate_type_handles((th, t))? {
96+
max_expr.check_valid_for(global_expressions)?;
97+
// Since `t` refers to `max_expr`, if we want our invariants to
98+
// remain true, we must advance `global_exprs_iter` beyond
99+
// `max_expr`.
100+
while let Some((eh, e)) = global_exprs_iter.next_if(|&(eh, _)| eh <= max_expr) {
101+
if let Some(max_type) =
102+
Self::validate_const_expression_handles((eh, e), constants, overrides)?
103+
{
104+
// Show that `eh` refers only to previously visited types.
105+
th.check_dep(max_type)?;
106+
}
107+
// We've advanced `global_exprs_iter` past `eh` already. But
108+
// since we now know that `eh` refers only to previously
109+
// visited types, it is again true that all expressions
110+
// behind `global_exprs_iter` refer only to previously
111+
// visited types. So we can continue to the next expression.
71112
}
72113
}
114+
115+
// Here we know that if `th` refers to any expressions at all,
116+
// `max_expr` is the latest one. And we know that `max_expr` is
117+
// behind `global_exprs_iter`. So `th` refers only to expressions
118+
// behind `global_exprs_iter`, and the invariants will still be
119+
// true on the next iteration.
73120
}
74121

75-
for handle_and_expr in global_expressions.iter() {
76-
Self::validate_const_expression_handles(handle_and_expr, constants, overrides, types)?;
122+
// Since we also enforced the usual intra-arena rules that expressions
123+
// refer only to prior expressions, expressions can only form cycles if
124+
// they include types. But we've shown that all types are acyclic, so
125+
// all expressions must be acyclic as well.
126+
//
127+
// Validate the remaining expressions normally.
128+
for handle_and_expr in global_exprs_iter {
129+
Self::validate_const_expression_handles(handle_and_expr, constants, overrides)?;
77130
}
78131

79132
let validate_type = |handle| Self::validate_type_handle(handle, types);
@@ -239,38 +292,86 @@ impl super::Validator {
239292
handle.check_valid_for(functions).map(|_| ())
240293
}
241294

295+
/// Validate all handles that occur in `ty`, whose handle is `handle`.
296+
///
297+
/// If `ty` refers to any expressions, return the highest-indexed expression
298+
/// handle that it uses. This is used for detecting cycles between the
299+
/// expression and type arenas.
300+
fn validate_type_handles(
301+
(handle, ty): (Handle<crate::Type>, &crate::Type),
302+
) -> Result<Option<Handle<crate::Expression>>, InvalidHandleError> {
303+
let max_expr = match ty.inner {
304+
crate::TypeInner::Scalar { .. }
305+
| crate::TypeInner::Vector { .. }
306+
| crate::TypeInner::Matrix { .. }
307+
| crate::TypeInner::ValuePointer { .. }
308+
| crate::TypeInner::Atomic { .. }
309+
| crate::TypeInner::Image { .. }
310+
| crate::TypeInner::Sampler { .. }
311+
| crate::TypeInner::AccelerationStructure
312+
| crate::TypeInner::RayQuery => None,
313+
crate::TypeInner::Pointer { base, space: _ } => {
314+
handle.check_dep(base)?;
315+
None
316+
}
317+
crate::TypeInner::Array { base, size, .. }
318+
| crate::TypeInner::BindingArray { base, size, .. } => {
319+
handle.check_dep(base)?;
320+
match size {
321+
crate::ArraySize::Pending(pending) => match pending {
322+
crate::PendingArraySize::Expression(expr) => Some(expr),
323+
crate::PendingArraySize::Override(_) => None,
324+
},
325+
crate::ArraySize::Constant(_) | crate::ArraySize::Dynamic => None,
326+
}
327+
}
328+
crate::TypeInner::Struct {
329+
ref members,
330+
span: _,
331+
} => {
332+
handle.check_dep_iter(members.iter().map(|m| m.ty))?;
333+
None
334+
}
335+
};
336+
337+
Ok(max_expr)
338+
}
339+
340+
/// Validate all handles that occur in `expression`, whose handle is `handle`.
341+
///
342+
/// If `expression` refers to any `Type`s, return the highest-indexed type
343+
/// handle that it uses. This is used for detecting cycles between the
344+
/// expression and type arenas.
242345
fn validate_const_expression_handles(
243346
(handle, expression): (Handle<crate::Expression>, &crate::Expression),
244347
constants: &Arena<crate::Constant>,
245348
overrides: &Arena<crate::Override>,
246-
types: &UniqueArena<crate::Type>,
247-
) -> Result<(), InvalidHandleError> {
349+
) -> Result<Option<Handle<crate::Type>>, InvalidHandleError> {
248350
let validate_constant = |handle| Self::validate_constant_handle(handle, constants);
249351
let validate_override = |handle| Self::validate_override_handle(handle, overrides);
250-
let validate_type = |handle| Self::validate_type_handle(handle, types);
251352

252-
match *expression {
253-
crate::Expression::Literal(_) => {}
353+
let max_type = match *expression {
354+
crate::Expression::Literal(_) => None,
254355
crate::Expression::Constant(constant) => {
255356
validate_constant(constant)?;
256357
handle.check_dep(constants[constant].init)?;
358+
None
257359
}
258360
crate::Expression::Override(override_) => {
259361
validate_override(override_)?;
260362
if let Some(init) = overrides[override_].init {
261363
handle.check_dep(init)?;
262364
}
365+
None
263366
}
264-
crate::Expression::ZeroValue(ty) => {
265-
validate_type(ty)?;
266-
}
367+
crate::Expression::ZeroValue(ty) => Some(ty),
267368
crate::Expression::Compose { ty, ref components } => {
268-
validate_type(ty)?;
269369
handle.check_dep_iter(components.iter().copied())?;
370+
Some(ty)
270371
}
271-
_ => {}
272-
}
273-
Ok(())
372+
_ => None,
373+
};
374+
Ok(max_type)
274375
}
275376

276377
#[allow(clippy::too_many_arguments)]
@@ -783,8 +884,47 @@ fn constant_deps() {
783884
handle_and_expr,
784885
&constants,
785886
&overrides,
786-
&types,
787887
)
788888
.is_err());
789889
}
790890
}
891+
892+
#[test]
893+
fn override_deps() {
894+
use super::Validator;
895+
use crate::{ArraySize, Expression, PendingArraySize, Scalar, Span, Type, TypeInner};
896+
897+
let nowhere = Span::default();
898+
899+
let mut m = crate::Module::default();
900+
901+
let ty_u32 = m.types.insert(
902+
Type {
903+
name: Some("u32".to_string()),
904+
inner: TypeInner::Scalar(Scalar::U32),
905+
},
906+
nowhere,
907+
);
908+
let ex_zero = m
909+
.global_expressions
910+
.append(Expression::ZeroValue(ty_u32), nowhere);
911+
let ty_arr = m.types.insert(
912+
Type {
913+
name: Some("bad_array".to_string()),
914+
inner: TypeInner::Array {
915+
base: ty_u32,
916+
size: ArraySize::Pending(PendingArraySize::Expression(ex_zero)),
917+
stride: 4,
918+
},
919+
},
920+
nowhere,
921+
);
922+
923+
// Everything should be okay now.
924+
assert!(Validator::validate_module_handles(&m).is_ok());
925+
926+
// Mutate `ex_zero`'s type to `ty_arr`, introducing a cycle.
927+
// Validation should catch the cycle.
928+
m.global_expressions[ex_zero] = Expression::ZeroValue(ty_arr);
929+
assert!(Validator::validate_module_handles(&m).is_err());
930+
}

0 commit comments

Comments
 (0)