From 96445a9217119698a9d0be6d29919a41312a41f2 Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Thu, 19 Dec 2024 18:19:39 -0800 Subject: [PATCH 1/2] [naga] Move type handle validation into its own function. --- naga/src/valid/handles.rs | 57 ++++++++++++++++++++++----------------- 1 file changed, 32 insertions(+), 25 deletions(-) diff --git a/naga/src/valid/handles.rs b/naga/src/valid/handles.rs index be4eb3dbac..2a342a0118 100644 --- a/naga/src/valid/handles.rs +++ b/naga/src/valid/handles.rs @@ -45,31 +45,8 @@ impl super::Validator { } = module; // NOTE: Types being first is important. All other forms of validation depend on this. - for (this_handle, ty) in types.iter() { - match ty.inner { - crate::TypeInner::Scalar { .. } - | crate::TypeInner::Vector { .. } - | crate::TypeInner::Matrix { .. } - | crate::TypeInner::ValuePointer { .. } - | crate::TypeInner::Atomic { .. } - | crate::TypeInner::Image { .. } - | crate::TypeInner::Sampler { .. } - | crate::TypeInner::AccelerationStructure - | crate::TypeInner::RayQuery => (), - crate::TypeInner::Pointer { base, space: _ } => { - this_handle.check_dep(base)?; - } - crate::TypeInner::Array { base, .. } - | crate::TypeInner::BindingArray { base, .. } => { - this_handle.check_dep(base)?; - } - crate::TypeInner::Struct { - ref members, - span: _, - } => { - this_handle.check_dep_iter(members.iter().map(|m| m.ty))?; - } - } + for handle_and_type in types.iter() { + Self::validate_type_handles(handle_and_type)?; } for handle_and_expr in global_expressions.iter() { @@ -239,6 +216,36 @@ impl super::Validator { handle.check_valid_for(functions).map(|_| ()) } + fn validate_type_handles( + (handle, ty): (Handle, &crate::Type), + ) -> Result<(), InvalidHandleError> { + match ty.inner { + crate::TypeInner::Scalar { .. } + | crate::TypeInner::Vector { .. } + | crate::TypeInner::Matrix { .. } + | crate::TypeInner::ValuePointer { .. } + | crate::TypeInner::Atomic { .. } + | crate::TypeInner::Image { .. } + | crate::TypeInner::Sampler { .. } + | crate::TypeInner::AccelerationStructure + | crate::TypeInner::RayQuery => (), + crate::TypeInner::Pointer { base, space: _ } => { + handle.check_dep(base)?; + } + crate::TypeInner::Array { base, .. } | crate::TypeInner::BindingArray { base, .. } => { + handle.check_dep(base)?; + } + crate::TypeInner::Struct { + ref members, + span: _, + } => { + handle.check_dep_iter(members.iter().map(|m| m.ty))?; + } + } + + Ok(()) + } + fn validate_const_expression_handles( (handle, expression): (Handle, &crate::Expression), constants: &Arena, From 75a55650481c54d984a53ffa47acb1112ff7f9ee Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Fri, 20 Dec 2024 18:20:43 -0800 Subject: [PATCH 2/2] [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. --- naga/src/valid/handles.rs | 181 +++++++++++++++++++++++++++++++++----- 1 file changed, 157 insertions(+), 24 deletions(-) diff --git a/naga/src/valid/handles.rs b/naga/src/valid/handles.rs index 2a342a0118..f0197a954a 100644 --- a/naga/src/valid/handles.rs +++ b/naga/src/valid/handles.rs @@ -44,13 +44,89 @@ impl super::Validator { ref diagnostic_filter_leaf, } = module; - // NOTE: Types being first is important. All other forms of validation depend on this. - for handle_and_type in types.iter() { - Self::validate_type_handles(handle_and_type)?; + // Because types can refer to global expressions and vice versa, to + // ensure the overall structure is free of cycles, we must traverse them + // both in tandem. + // + // Try to visit all types and global expressions in an order such that + // each item refers only to previously visited items. If we succeed, + // that shows that there cannot be any cycles, since walking any edge + // advances you towards the beginning of the visiting order. + // + // Validate all the handles in types and expressions as we traverse the + // arenas. + let mut global_exprs_iter = global_expressions.iter().peekable(); + for (th, t) in types.iter() { + // Imagine the `for` loop and `global_exprs_iter` as two fingers + // walking the type and global expression arenas. They don't visit + // elements at the same rate: sometimes one processes a bunch of + // elements while the other one stays still. But at each point, they + // check that the two ranges of elements they've visited only refer + // to other elements in those ranges. + // + // For brevity, we'll say 'handles behind `global_exprs_iter`' to + // mean handles that have already been produced by + // `global_exprs_iter`. Once `global_exprs_iter` returns `None`, all + // global expression handles are 'behind' it. + // + // At this point: + // + // - All types visited by prior iterations (that is, before + // `th`/`t`) refer only to expressions behind `global_exprs_iter`. + // + // On the first iteration, this is obviously true: there are no + // prior iterations, and `global_exprs_iter` hasn't produced + // anything yet. At the bottom of the loop, we'll claim that it's + // true for `th`/`t` as well, so the condition remains true when + // we advance to the next type. + // + // - All expressions behind `global_exprs_iter` refer only to + // previously visited types. + // + // Again, trivially true at the start, and we'll show it's true + // about each expression that `global_exprs_iter` produces. + // + // Once we also check that arena elements only refer to prior + // elements in that arena, we can see that `th`/`t` does not + // participate in a cycle: it only refers to previously visited + // types and expressions behind `global_exprs_iter`, and none of + // those refer to `th`/`t`, because they passed the same checks + // before we reached `th`/`t`. + if let Some(max_expr) = Self::validate_type_handles((th, t))? { + max_expr.check_valid_for(global_expressions)?; + // Since `t` refers to `max_expr`, if we want our invariants to + // remain true, we must advance `global_exprs_iter` beyond + // `max_expr`. + while let Some((eh, e)) = global_exprs_iter.next_if(|&(eh, _)| eh <= max_expr) { + if let Some(max_type) = + Self::validate_const_expression_handles((eh, e), constants, overrides)? + { + // Show that `eh` refers only to previously visited types. + th.check_dep(max_type)?; + } + // We've advanced `global_exprs_iter` past `eh` already. But + // since we now know that `eh` refers only to previously + // visited types, it is again true that all expressions + // behind `global_exprs_iter` refer only to previously + // visited types. So we can continue to the next expression. + } + } + + // Here we know that if `th` refers to any expressions at all, + // `max_expr` is the latest one. And we know that `max_expr` is + // behind `global_exprs_iter`. So `th` refers only to expressions + // behind `global_exprs_iter`, and the invariants will still be + // true on the next iteration. } - for handle_and_expr in global_expressions.iter() { - Self::validate_const_expression_handles(handle_and_expr, constants, overrides, types)?; + // Since we also enforced the usual intra-arena rules that expressions + // refer only to prior expressions, expressions can only form cycles if + // they include types. But we've shown that all types are acyclic, so + // all expressions must be acyclic as well. + // + // Validate the remaining expressions normally. + for handle_and_expr in global_exprs_iter { + Self::validate_const_expression_handles(handle_and_expr, constants, overrides)?; } let validate_type = |handle| Self::validate_type_handle(handle, types); @@ -216,10 +292,15 @@ impl super::Validator { handle.check_valid_for(functions).map(|_| ()) } + /// Validate all handles that occur in `ty`, whose handle is `handle`. + /// + /// If `ty` refers to any expressions, return the highest-indexed expression + /// handle that it uses. This is used for detecting cycles between the + /// expression and type arenas. fn validate_type_handles( (handle, ty): (Handle, &crate::Type), - ) -> Result<(), InvalidHandleError> { - match ty.inner { + ) -> Result>, InvalidHandleError> { + let max_expr = match ty.inner { crate::TypeInner::Scalar { .. } | crate::TypeInner::Vector { .. } | crate::TypeInner::Matrix { .. } @@ -228,56 +309,69 @@ impl super::Validator { | crate::TypeInner::Image { .. } | crate::TypeInner::Sampler { .. } | crate::TypeInner::AccelerationStructure - | crate::TypeInner::RayQuery => (), + | crate::TypeInner::RayQuery => None, crate::TypeInner::Pointer { base, space: _ } => { handle.check_dep(base)?; + None } - crate::TypeInner::Array { base, .. } | crate::TypeInner::BindingArray { base, .. } => { + crate::TypeInner::Array { base, size, .. } + | crate::TypeInner::BindingArray { base, size, .. } => { handle.check_dep(base)?; + match size { + crate::ArraySize::Pending(pending) => match pending { + crate::PendingArraySize::Expression(expr) => Some(expr), + crate::PendingArraySize::Override(_) => None, + }, + crate::ArraySize::Constant(_) | crate::ArraySize::Dynamic => None, + } } crate::TypeInner::Struct { ref members, span: _, } => { handle.check_dep_iter(members.iter().map(|m| m.ty))?; + None } - } + }; - Ok(()) + Ok(max_expr) } + /// Validate all handles that occur in `expression`, whose handle is `handle`. + /// + /// If `expression` refers to any `Type`s, return the highest-indexed type + /// handle that it uses. This is used for detecting cycles between the + /// expression and type arenas. fn validate_const_expression_handles( (handle, expression): (Handle, &crate::Expression), constants: &Arena, overrides: &Arena, - types: &UniqueArena, - ) -> Result<(), InvalidHandleError> { + ) -> Result>, InvalidHandleError> { let validate_constant = |handle| Self::validate_constant_handle(handle, constants); let validate_override = |handle| Self::validate_override_handle(handle, overrides); - let validate_type = |handle| Self::validate_type_handle(handle, types); - match *expression { - crate::Expression::Literal(_) => {} + let max_type = match *expression { + crate::Expression::Literal(_) => None, crate::Expression::Constant(constant) => { validate_constant(constant)?; handle.check_dep(constants[constant].init)?; + None } crate::Expression::Override(override_) => { validate_override(override_)?; if let Some(init) = overrides[override_].init { handle.check_dep(init)?; } + None } - crate::Expression::ZeroValue(ty) => { - validate_type(ty)?; - } + crate::Expression::ZeroValue(ty) => Some(ty), crate::Expression::Compose { ty, ref components } => { - validate_type(ty)?; handle.check_dep_iter(components.iter().copied())?; + Some(ty) } - _ => {} - } - Ok(()) + _ => None, + }; + Ok(max_type) } #[allow(clippy::too_many_arguments)] @@ -790,8 +884,47 @@ fn constant_deps() { handle_and_expr, &constants, &overrides, - &types, ) .is_err()); } } + +#[test] +fn override_deps() { + use super::Validator; + use crate::{ArraySize, Expression, PendingArraySize, Scalar, Span, Type, TypeInner}; + + let nowhere = Span::default(); + + let mut m = crate::Module::default(); + + let ty_u32 = m.types.insert( + Type { + name: Some("u32".to_string()), + inner: TypeInner::Scalar(Scalar::U32), + }, + nowhere, + ); + let ex_zero = m + .global_expressions + .append(Expression::ZeroValue(ty_u32), nowhere); + let ty_arr = m.types.insert( + Type { + name: Some("bad_array".to_string()), + inner: TypeInner::Array { + base: ty_u32, + size: ArraySize::Pending(PendingArraySize::Expression(ex_zero)), + stride: 4, + }, + }, + nowhere, + ); + + // Everything should be okay now. + assert!(Validator::validate_module_handles(&m).is_ok()); + + // Mutate `ex_zero`'s type to `ty_arr`, introducing a cycle. + // Validation should catch the cycle. + m.global_expressions[ex_zero] = Expression::ZeroValue(ty_arr); + assert!(Validator::validate_module_handles(&m).is_err()); +}