From 60a26308ae3e84cf423e24c6ccd856825a625a19 Mon Sep 17 00:00:00 2001
From: Jeff Young <jeff@rokeby.ie>
Date: Sat, 29 Mar 2025 21:23:07 +0000
Subject: [PATCH] COMMIT lifetime safety.
If we pass a COMMIT to a posted action, the COMMIT
may no longer exist when the action is run.
Equally problematic, if another COMMIT is pushed
in between we'd probably also run into trouble.
We still allow the API to do this because we don't
have a better solution at present. But we need
one.
---
common/tool/tool_manager.cpp | 33 ++++++++++++++++++++-------------
include/tool/tool_manager.h | 14 +++++++++-----
pcbnew/api/api_handler_pcb.cpp | 2 +-
3 files changed, 30 insertions(+), 19 deletions(-)
diff --git a/common/tool/tool_manager.cpp b/common/tool/tool_manager.cpp
index a4088035b4..e05b61a6ff 100644
--- a/common/tool/tool_manager.cpp
+++ b/common/tool/tool_manager.cpp
@@ -328,7 +328,7 @@ VECTOR2D TOOL_MANAGER::GetCursorPosition() const
bool TOOL_MANAGER::doRunAction( const TOOL_ACTION& aAction, bool aNow, const ki::any& aParam,
- COMMIT* aCommit )
+ COMMIT* aCommit, bool aFromAPI )
{
if( m_shuttingDown )
return true;
@@ -336,10 +336,6 @@ bool TOOL_MANAGER::doRunAction( const TOOL_ACTION& aAction, bool aNow, const ki:
bool retVal = false;
TOOL_EVENT event = aAction.MakeEvent();
- // We initialize the SYNCHRONOUS state to finished so that tools that don't have an event
- // loop won't hang if someone forgets to set the state.
- std::atomic<SYNCRONOUS_TOOL_STATE> synchronousControl = STS_FINISHED;
-
if( event.Category() == TC_COMMAND )
event.SetMousePosition( GetCursorPosition() );
@@ -347,20 +343,20 @@ bool TOOL_MANAGER::doRunAction( const TOOL_ACTION& aAction, bool aNow, const ki:
if( aParam.has_value() )
event.SetParameter( aParam );
- // Pass the commit (if any)
- if( aCommit )
- {
- event.SetSynchronous( &synchronousControl );
- event.SetCommit( aCommit );
- }
-
if( aNow )
{
TOOL_STATE* current = m_activeState;
+ // An event with a commit must be run synchronously
if( aCommit )
{
- // An event with a commit must be run synchronously
+ // We initialize the SYNCHRONOUS state to finished so that tools that don't have an
+ // event loop won't hang if someone forgets to set the state.
+ std::atomic<SYNCRONOUS_TOOL_STATE> synchronousControl = STS_FINISHED;
+
+ event.SetSynchronous( &synchronousControl );
+ event.SetCommit( aCommit );
+
processEvent( event );
while( synchronousControl == STS_RUNNING )
@@ -384,6 +380,17 @@ bool TOOL_MANAGER::doRunAction( const TOOL_ACTION& aAction, bool aNow, const ki:
}
else
{
+ // It is really dangerous to pass a commit (whose lifetime we can't guarantee) to
+ // deferred event processing. There is a possibility that user actions will get run
+ // in between, which might either affect the lifetime of the commit or push or pop
+ // other commits. However, we don't currently have a better solution for the API.
+ if( aCommit )
+ {
+ wxASSERT_MSG( aFromAPI, wxT( "Deferred actions have no way of guaranteeing the "
+ "lifetime of the COMMIT object" ) );
+ event.SetCommit( aCommit );
+ }
+
PostEvent( event );
}
diff --git a/include/tool/tool_manager.h b/include/tool/tool_manager.h
index 4fd7070981..1d0ce4b90d 100644
--- a/include/tool/tool_manager.h
+++ b/include/tool/tool_manager.h
@@ -256,9 +256,10 @@ public:
*
* @param aAction is the action to be invoked.
* @param aParam is an optional parameter that might be used by the invoked action. Its meaning
- * depends on the action.
+ * depends on the action. The parameter is not allowed to be a COMMIT as we have
+ * no way of guaranteeing the lifetime of the COMMIT object.
*/
- template<typename T>
+ template<typename T, std::enable_if_t<!std::is_convertible_v<T, COMMIT*>>* = nullptr>
bool PostAction( const TOOL_ACTION& aAction, T aParam )
{
// Use a cast to ensure the proper type is stored inside the parameter
@@ -275,12 +276,15 @@ public:
doRunAction( aAction, false, a, nullptr );
}
- bool PostAction( const TOOL_ACTION& aAction, COMMIT* aCommit )
+ // TODO: we need a way to guarantee that user events can't be processed in
+ // between API actions. Otherwise the COMMITs will get tangled up and we'll
+ // crash on bad pointers.
+ bool PostAPIAction( const TOOL_ACTION& aAction, COMMIT* aCommit )
{
// Default initialize the parameter argument to an empty ki_any
ki::any a;
- return doRunAction( aAction, false, a, aCommit );
+ return doRunAction( aAction, false, a, aCommit, true );
}
/**
@@ -538,7 +542,7 @@ private:
* Helper function to actually run an action.
*/
bool doRunAction( const TOOL_ACTION& aAction, bool aNow, const ki::any& aParam,
- COMMIT* aCommit );
+ COMMIT* aCommit, bool aFromAPI = false );
bool doRunAction( const std::string& aActionName, bool aNow, const ki::any& aParam,
COMMIT* aCommit );
diff --git a/pcbnew/api/api_handler_pcb.cpp b/pcbnew/api/api_handler_pcb.cpp
index 4e44b49b91..d1c313db3a 100644
--- a/pcbnew/api/api_handler_pcb.cpp
+++ b/pcbnew/api/api_handler_pcb.cpp
@@ -1138,7 +1138,7 @@ HANDLER_RESULT<Empty> API_HANDLER_PCB::handleInteractiveMoveItems(
mgr->RunAction<EDA_ITEMS*>( PCB_ACTIONS::selectItems, &toSelect );
COMMIT* commit = getCurrentCommit( aCtx.ClientName );
- mgr->PostAction( PCB_ACTIONS::move, commit );
+ mgr->PostAPIAction( PCB_ACTIONS::move, commit );
return Empty();
}