From b9e146a37cbbc0c013c40dde01cf2e70ff2d4460 Mon Sep 17 00:00:00 2001 From: Tobias Koppers Date: Thu, 5 Dec 2024 13:59:33 +0100 Subject: [PATCH 1/3] fix incorrect task access --- .../crates/turbo-tasks-backend/Cargo.toml | 1 + .../turbo-tasks-backend/src/backend/mod.rs | 2 +- .../src/backend/operation/connect_child.rs | 2 +- .../src/backend/operation/mod.rs | 44 +++++++++ .../src/backend/operation/update_output.rs | 4 +- .../crates/turbo-tasks-backend/src/data.rs | 97 +++++++++++++------ 6 files changed, 116 insertions(+), 34 deletions(-) diff --git a/turbopack/crates/turbo-tasks-backend/Cargo.toml b/turbopack/crates/turbo-tasks-backend/Cargo.toml index ff404a28baf17..5a644df8a83ec 100644 --- a/turbopack/crates/turbo-tasks-backend/Cargo.toml +++ b/turbopack/crates/turbo-tasks-backend/Cargo.toml @@ -15,6 +15,7 @@ workspace = true [features] default = [] verify_serialization = [] +verify_task_access = [] trace_aggregation_update = [] trace_find_and_schedule = [] lmdb = ["dep:lmdb-rkv"] diff --git a/turbopack/crates/turbo-tasks-backend/src/backend/mod.rs b/turbopack/crates/turbo-tasks-backend/src/backend/mod.rs index e6c3d88e59288..46b42f4b4b086 100644 --- a/turbopack/crates/turbo-tasks-backend/src/backend/mod.rs +++ b/turbopack/crates/turbo-tasks-backend/src/backend/mod.rs @@ -1592,7 +1592,7 @@ impl TurboTasksBackendInner { let mut ctx = self.execute_context(turbo_tasks); let mut collectibles = AutoMap::default(); { - let mut task = ctx.task(task_id, TaskDataCategory::Data); + let mut task = ctx.task(task_id, TaskDataCategory::All); // Ensure it's an root node loop { let aggregation_number = get_aggregation_number(&task); diff --git a/turbopack/crates/turbo-tasks-backend/src/backend/operation/connect_child.rs b/turbopack/crates/turbo-tasks-backend/src/backend/operation/connect_child.rs index a33300138c231..e6befd263a406 100644 --- a/turbopack/crates/turbo-tasks-backend/src/backend/operation/connect_child.rs +++ b/turbopack/crates/turbo-tasks-backend/src/backend/operation/connect_child.rs @@ -126,7 +126,7 @@ impl ConnectChildOperation { drop(parent_task); { - let mut task = ctx.task(child_task_id, TaskDataCategory::Data); + let mut task = ctx.task(child_task_id, TaskDataCategory::Meta); if !task.has_key(&CachedDataItemKey::Output {}) { let description = ctx.get_task_desc_fn(child_task_id); let should_schedule = task.add(CachedDataItem::new_scheduled(description)); diff --git a/turbopack/crates/turbo-tasks-backend/src/backend/operation/mod.rs b/turbopack/crates/turbo-tasks-backend/src/backend/operation/mod.rs index cf9f7c9cc83e5..5ae9153c2b8cd 100644 --- a/turbopack/crates/turbo-tasks-backend/src/backend/operation/mod.rs +++ b/turbopack/crates/turbo-tasks-backend/src/backend/operation/mod.rs @@ -204,6 +204,8 @@ where task, task_id, backend: self.backend, + #[cfg(feature = "verify_task_access")] + category, } } @@ -258,11 +260,15 @@ where task: task1, task_id: task_id1, backend: self.backend, + #[cfg(feature = "verify_task_access")] + category, }, TaskGuardImpl { task: task2, task_id: task_id2, backend: self.backend, + #[cfg(feature = "verify_task_access")] + category, }, ) } @@ -385,6 +391,34 @@ struct TaskGuardImpl<'a, B: BackingStorage> { task_id: TaskId, task: StorageWriteGuard<'a, TaskId, CachedDataItem>, backend: &'a TurboTasksBackendInner, + #[cfg(feature = "verify_task_access")] + category: TaskDataCategory, +} + +impl TaskGuardImpl<'_, B> { + #[inline] + fn check_access(&self, category: TaskDataCategory) { + #[cfg(feature = "verify_task_access")] + { + match category { + TaskDataCategory::All => { + // This category is used for non-persisted data + } + TaskDataCategory::Data => { + assert!( + self.category == TaskDataCategory::Data + || self.category == TaskDataCategory::All + ); + } + TaskDataCategory::Meta => { + assert!( + self.category == TaskDataCategory::Meta + || self.category == TaskDataCategory::All + ); + } + } + } + } } impl Debug for TaskGuardImpl<'_, B> { @@ -408,6 +442,7 @@ impl TaskGuard for TaskGuardImpl<'_, B> { #[must_use] fn add(&mut self, item: CachedDataItem) -> bool { + self.check_access(item.category()); if !self.backend.should_persist() || self.task_id.is_transient() || !item.is_persistent() { self.task.add(item) } else if self.task.add(item.clone()) { @@ -424,11 +459,13 @@ impl TaskGuard for TaskGuardImpl<'_, B> { } fn add_new(&mut self, item: CachedDataItem) { + self.check_access(item.category()); let added = self.add(item); assert!(added, "Item already exists"); } fn insert(&mut self, item: CachedDataItem) -> Option { + self.check_access(item.category()); let (key, value) = item.into_key_and_value(); if !self.backend.should_persist() || self.task_id.is_transient() || !key.is_persistent() { self.task @@ -472,6 +509,7 @@ impl TaskGuard for TaskGuardImpl<'_, B> { key: &CachedDataItemKey, update: impl FnOnce(Option) -> Option, ) { + self.check_access(key.category()); if !self.backend.should_persist() || self.task_id.is_transient() || !key.is_persistent() { self.task.update(key, update); return; @@ -480,6 +518,8 @@ impl TaskGuard for TaskGuardImpl<'_, B> { task, task_id, backend, + #[cfg(feature = "verify_task_access")] + category: _, } = self; let mut add_persisting_item = false; task.update(key, |old| { @@ -519,6 +559,7 @@ impl TaskGuard for TaskGuardImpl<'_, B> { } fn remove(&mut self, key: &CachedDataItemKey) -> Option { + self.check_access(key.category()); let old_value = self.task.remove(key); if let Some(value) = old_value { if self.backend.should_persist() @@ -545,14 +586,17 @@ impl TaskGuard for TaskGuardImpl<'_, B> { } fn get(&self, key: &CachedDataItemKey) -> Option<&CachedDataItemValue> { + self.check_access(key.category()); self.task.get(key) } fn get_mut(&mut self, key: &CachedDataItemKey) -> Option<&mut CachedDataItemValue> { + self.check_access(key.category()); self.task.get_mut(key) } fn has_key(&self, key: &CachedDataItemKey) -> bool { + self.check_access(key.category()); self.task.has_key(key) } diff --git a/turbopack/crates/turbo-tasks-backend/src/backend/operation/update_output.rs b/turbopack/crates/turbo-tasks-backend/src/backend/operation/update_output.rs index 48c925aae2ab8..fc36d911c4a8a 100644 --- a/turbopack/crates/turbo-tasks-backend/src/backend/operation/update_output.rs +++ b/turbopack/crates/turbo-tasks-backend/src/backend/operation/update_output.rs @@ -43,7 +43,7 @@ impl UpdateOutputOperation { output: Result, Option>>, mut ctx: impl ExecuteContext, ) { - let mut task = ctx.task(task_id, TaskDataCategory::Data); + let mut task = ctx.task(task_id, TaskDataCategory::Meta); if let Some(InProgressState::InProgress { stale: true, .. }) = get!(task, InProgress) { // Skip updating the output when the task is stale return; @@ -173,7 +173,7 @@ impl Operation for UpdateOutputOperation { ref mut queue, } => { if let Some(child_id) = children.pop() { - let mut child_task = ctx.task(child_id, TaskDataCategory::Data); + let mut child_task = ctx.task(child_id, TaskDataCategory::Meta); if !child_task.has_key(&CachedDataItemKey::Output {}) { make_task_dirty_internal( &mut child_task, diff --git a/turbopack/crates/turbo-tasks-backend/src/data.rs b/turbopack/crates/turbo-tasks-backend/src/data.rs index 86f0ad6d9ae50..0eb709750e63b 100644 --- a/turbopack/crates/turbo-tasks-backend/src/data.rs +++ b/turbopack/crates/turbo-tasks-backend/src/data.rs @@ -496,6 +496,42 @@ impl CachedDataItem { listener, ) } + + pub fn category(&self) -> TaskDataCategory { + match self { + Self::Collectible { .. } + | Self::Child { .. } + | Self::ChildrenCount { .. } + | Self::CellData { .. } + | Self::CellTypeMaxIndex { .. } + | Self::OutputDependency { .. } + | Self::CellDependency { .. } + | Self::CollectiblesDependency { .. } + | Self::OutputDependent { .. } + | Self::CellDependent { .. } + | Self::CollectiblesDependent { .. } => TaskDataCategory::Data, + + Self::Output { .. } + | Self::AggregationNumber { .. } + | Self::Dirty { .. } + | Self::Follower { .. } + | Self::Upper { .. } + | Self::PersistentUpperCount { .. } + | Self::AggregatedDirtyContainer { .. } + | Self::AggregatedCollectible { .. } + | Self::AggregatedDirtyContainerCount { .. } => TaskDataCategory::Meta, + + Self::OutdatedCollectible { .. } + | Self::OutdatedOutputDependency { .. } + | Self::OutdatedCellDependency { .. } + | Self::OutdatedCollectiblesDependency { .. } + | Self::OutdatedChild { .. } + | Self::InProgressCell { .. } + | Self::InProgress { .. } + | Self::Error { .. } + | Self::AggregateRoot { .. } => TaskDataCategory::All, + } + } } impl CachedDataItemKey { @@ -543,36 +579,37 @@ impl CachedDataItemKey { pub fn category(&self) -> TaskDataCategory { match self { - CachedDataItemKey::Collectible { .. } - | CachedDataItemKey::Child { .. } - | CachedDataItemKey::ChildrenCount { .. } - | CachedDataItemKey::CellData { .. } - | CachedDataItemKey::CellTypeMaxIndex { .. } - | CachedDataItemKey::OutputDependency { .. } - | CachedDataItemKey::CellDependency { .. } - | CachedDataItemKey::CollectiblesDependency { .. } - | CachedDataItemKey::OutputDependent { .. } - | CachedDataItemKey::CellDependent { .. } - | CachedDataItemKey::CollectiblesDependent { .. } - | CachedDataItemKey::InProgress { .. } - | CachedDataItemKey::InProgressCell { .. } - | CachedDataItemKey::OutdatedCollectible { .. } - | CachedDataItemKey::OutdatedOutputDependency { .. } - | CachedDataItemKey::OutdatedCellDependency { .. } - | CachedDataItemKey::OutdatedCollectiblesDependency { .. } - | CachedDataItemKey::OutdatedChild { .. } - | CachedDataItemKey::Error { .. } => TaskDataCategory::Data, - - CachedDataItemKey::Output { .. } - | CachedDataItemKey::AggregationNumber { .. } - | CachedDataItemKey::Dirty { .. } - | CachedDataItemKey::Follower { .. } - | CachedDataItemKey::Upper { .. } - | CachedDataItemKey::PersistentUpperCount { .. } - | CachedDataItemKey::AggregatedDirtyContainer { .. } - | CachedDataItemKey::AggregatedCollectible { .. } - | CachedDataItemKey::AggregatedDirtyContainerCount { .. } - | CachedDataItemKey::AggregateRoot { .. } => TaskDataCategory::Meta, + Self::Collectible { .. } + | Self::Child { .. } + | Self::ChildrenCount { .. } + | Self::CellData { .. } + | Self::CellTypeMaxIndex { .. } + | Self::OutputDependency { .. } + | Self::CellDependency { .. } + | Self::CollectiblesDependency { .. } + | Self::OutputDependent { .. } + | Self::CellDependent { .. } + | Self::CollectiblesDependent { .. } => TaskDataCategory::Data, + + Self::Output { .. } + | Self::AggregationNumber { .. } + | Self::Dirty { .. } + | Self::Follower { .. } + | Self::Upper { .. } + | Self::PersistentUpperCount { .. } + | Self::AggregatedDirtyContainer { .. } + | Self::AggregatedCollectible { .. } + | Self::AggregatedDirtyContainerCount { .. } => TaskDataCategory::Meta, + + Self::OutdatedCollectible { .. } + | Self::OutdatedOutputDependency { .. } + | Self::OutdatedCellDependency { .. } + | Self::OutdatedCollectiblesDependency { .. } + | Self::OutdatedChild { .. } + | Self::InProgressCell { .. } + | Self::InProgress { .. } + | Self::Error { .. } + | Self::AggregateRoot { .. } => TaskDataCategory::All, } } } From bad85da7d84b9231b6f1d1d04195e2212da58e88 Mon Sep 17 00:00:00 2001 From: Tobias Koppers Date: Thu, 5 Dec 2024 14:29:58 +0100 Subject: [PATCH 2/3] change to debug_assertions --- turbopack/crates/turbo-tasks-backend/Cargo.toml | 1 - .../src/backend/operation/mod.rs | 17 +++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/turbopack/crates/turbo-tasks-backend/Cargo.toml b/turbopack/crates/turbo-tasks-backend/Cargo.toml index 5a644df8a83ec..ff404a28baf17 100644 --- a/turbopack/crates/turbo-tasks-backend/Cargo.toml +++ b/turbopack/crates/turbo-tasks-backend/Cargo.toml @@ -15,7 +15,6 @@ workspace = true [features] default = [] verify_serialization = [] -verify_task_access = [] trace_aggregation_update = [] trace_find_and_schedule = [] lmdb = ["dep:lmdb-rkv"] diff --git a/turbopack/crates/turbo-tasks-backend/src/backend/operation/mod.rs b/turbopack/crates/turbo-tasks-backend/src/backend/operation/mod.rs index 5ae9153c2b8cd..1de09e95e737d 100644 --- a/turbopack/crates/turbo-tasks-backend/src/backend/operation/mod.rs +++ b/turbopack/crates/turbo-tasks-backend/src/backend/operation/mod.rs @@ -204,7 +204,7 @@ where task, task_id, backend: self.backend, - #[cfg(feature = "verify_task_access")] + #[cfg(debug_assertions)] category, } } @@ -260,14 +260,14 @@ where task: task1, task_id: task_id1, backend: self.backend, - #[cfg(feature = "verify_task_access")] + #[cfg(debug_assertions)] category, }, TaskGuardImpl { task: task2, task_id: task_id2, backend: self.backend, - #[cfg(feature = "verify_task_access")] + #[cfg(debug_assertions)] category, }, ) @@ -391,27 +391,28 @@ struct TaskGuardImpl<'a, B: BackingStorage> { task_id: TaskId, task: StorageWriteGuard<'a, TaskId, CachedDataItem>, backend: &'a TurboTasksBackendInner, - #[cfg(feature = "verify_task_access")] + #[cfg(debug_assertions)] category: TaskDataCategory, } impl TaskGuardImpl<'_, B> { + /// Verify that the task guard restored the correct category + /// before accessing the data. #[inline] fn check_access(&self, category: TaskDataCategory) { - #[cfg(feature = "verify_task_access")] { match category { TaskDataCategory::All => { // This category is used for non-persisted data } TaskDataCategory::Data => { - assert!( + debug_assert!( self.category == TaskDataCategory::Data || self.category == TaskDataCategory::All ); } TaskDataCategory::Meta => { - assert!( + debug_assert!( self.category == TaskDataCategory::Meta || self.category == TaskDataCategory::All ); @@ -518,7 +519,7 @@ impl TaskGuard for TaskGuardImpl<'_, B> { task, task_id, backend, - #[cfg(feature = "verify_task_access")] + #[cfg(debug_assertions)] category: _, } = self; let mut add_persisting_item = false; From d14254499e8fc8fe76a049ab085b83c9141ef3da Mon Sep 17 00:00:00 2001 From: Tobias Koppers Date: Thu, 5 Dec 2024 14:49:49 +0100 Subject: [PATCH 3/3] add conditional compilation --- .../crates/turbo-tasks-backend/src/backend/operation/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/turbopack/crates/turbo-tasks-backend/src/backend/operation/mod.rs b/turbopack/crates/turbo-tasks-backend/src/backend/operation/mod.rs index 1de09e95e737d..7b0c72b3e03e0 100644 --- a/turbopack/crates/turbo-tasks-backend/src/backend/operation/mod.rs +++ b/turbopack/crates/turbo-tasks-backend/src/backend/operation/mod.rs @@ -406,12 +406,14 @@ impl TaskGuardImpl<'_, B> { // This category is used for non-persisted data } TaskDataCategory::Data => { + #[cfg(debug_assertions)] debug_assert!( self.category == TaskDataCategory::Data || self.category == TaskDataCategory::All ); } TaskDataCategory::Meta => { + #[cfg(debug_assertions)] debug_assert!( self.category == TaskDataCategory::Meta || self.category == TaskDataCategory::All