From 31b788c04fe2db622fc73d097d0d049507f6a41e Mon Sep 17 00:00:00 2001
From: Jeff Young <jeff@rokeby.ie>
Date: Tue, 11 Mar 2025 15:08:13 +0000
Subject: [PATCH] Add a bit of protection from integer overflows.
Fixes https://gitlab.com/kicad/code/kicad/-/issues/19660
---
common/dialogs/panel_setup_netclasses.cpp | 38 +++++++++++++++++++----
include/board_design_settings.h | 2 ++
include/dialogs/panel_setup_netclasses.h | 1 +
pcbnew/board_design_settings.cpp | 3 +-
pcbnew/drc/drc_engine.cpp | 18 +++++++++++
5 files changed, 55 insertions(+), 7 deletions(-)
diff --git a/common/dialogs/panel_setup_netclasses.cpp b/common/dialogs/panel_setup_netclasses.cpp
index 9e4658c853..51beec93f4 100644
--- a/common/dialogs/panel_setup_netclasses.cpp
+++ b/common/dialogs/panel_setup_netclasses.cpp
@@ -35,6 +35,7 @@
#include <dialogs/panel_setup_netclasses.h>
#include <tool/tool_manager.h>
#include <pcb_painter.h>
+#include <board_design_settings.h>
#include <string_utils.h>
#include <view/view.h>
#include <widgets/grid_color_swatch_helpers.h>
@@ -504,16 +505,16 @@ bool PANEL_SETUP_NETCLASSES::TransferDataFromWindow()
wxASSERT_MSG( lineIdx >= 0, "Line style name not found." );
+ // clang-format off
nc->SetClearance( m_netclassGrid->GetOptionalUnitValue( aRow, GRID_CLEARANCE ) );
nc->SetTrackWidth( m_netclassGrid->GetOptionalUnitValue( aRow, GRID_TRACKSIZE ) );
nc->SetViaDiameter( m_netclassGrid->GetOptionalUnitValue( aRow, GRID_VIASIZE ) );
nc->SetViaDrill( m_netclassGrid->GetOptionalUnitValue( aRow, GRID_VIADRILL ) );
nc->SetuViaDiameter( m_netclassGrid->GetOptionalUnitValue( aRow, GRID_uVIASIZE ) );
nc->SetuViaDrill( m_netclassGrid->GetOptionalUnitValue( aRow, GRID_uVIADRILL ) );
- nc->SetDiffPairWidth(
- m_netclassGrid->GetOptionalUnitValue( aRow, GRID_DIFF_PAIR_WIDTH ) );
- nc->SetDiffPairGap( m_netclassGrid->GetOptionalUnitValue( aRow,
- GRID_DIFF_PAIR_GAP ) );
+ nc->SetDiffPairWidth( m_netclassGrid->GetOptionalUnitValue( aRow, GRID_DIFF_PAIR_WIDTH ) );
+ nc->SetDiffPairGap( m_netclassGrid->GetOptionalUnitValue( aRow, GRID_DIFF_PAIR_GAP ) );
+ // clang-format on
if( !nc->IsDefault() )
{
@@ -590,6 +591,26 @@ bool PANEL_SETUP_NETCLASSES::validateNetclassName( int aRow, const wxString& aNa
}
+bool PANEL_SETUP_NETCLASSES::validateNetclassClearance( int aRow )
+{
+ // Clip clearance. This is not a nag; we can end up with overflow errors and very poor
+ // performance if the clearance is too large.
+
+ std::optional<int> clearance = m_netclassGrid->GetOptionalUnitValue( aRow, GRID_CLEARANCE );
+
+ if( clearance.has_value() && clearance.value() > MAXIMUM_CLEARANCE )
+ {
+ wxString msg = wxString::Format( _( "Clearance was too large. It has been clipped to %s." ),
+ m_frame->StringFromValue( MAXIMUM_CLEARANCE, true ) );
+ PAGED_DIALOG::GetDialog( this )->SetError( msg, this, m_netclassGrid, aRow, GRID_CLEARANCE );
+ m_netclassGrid->SetUnitValue( aRow, GRID_CLEARANCE, MAXIMUM_CLEARANCE );
+ return false;
+ }
+
+ return true;
+}
+
+
void PANEL_SETUP_NETCLASSES::OnNetclassGridCellChanging( wxGridEvent& event )
{
if( event.GetCol() == GRID_NAME )
@@ -615,6 +636,10 @@ void PANEL_SETUP_NETCLASSES::OnNetclassGridCellChanging( wxGridEvent& event )
event.Veto();
}
}
+ else if( event.GetCol() == GRID_CLEARANCE )
+ {
+ validateNetclassClearance( event.GetRow() );
+ }
}
@@ -901,8 +926,6 @@ bool PANEL_SETUP_NETCLASSES::Validate()
if( !m_netclassGrid->CommitPendingChanges() || !m_assignmentGrid->CommitPendingChanges() )
return false;
- wxString msg;
-
// Test net class parameters.
for( int row = 0; row < m_netclassGrid->GetNumberRows(); row++ )
{
@@ -912,6 +935,9 @@ bool PANEL_SETUP_NETCLASSES::Validate()
if( !validateNetclassName( row, netclassName, false ) )
return false;
+
+ if( !validateNetclassClearance( row ) )
+ return false;
}
return true;
diff --git a/include/board_design_settings.h b/include/board_design_settings.h
index 5adb2a3eee..24a5720ecf 100644
--- a/include/board_design_settings.h
+++ b/include/board_design_settings.h
@@ -101,6 +101,8 @@
#define MINIMUM_ERROR_SIZE_MM 0.001 // For arc approximation
#define MAXIMUM_ERROR_SIZE_MM 0.1 // For arc approximation
+#define MAXIMUM_CLEARANCE pcbIUScale.mmToIU( 500 ) // to prevent int-overflows
+
// Min/max values used in dialogs to validate settings
#define MINIMUM_LINE_WIDTH_MM 0.005 // minimal line width entered in a dialog
#define MAXIMUM_LINE_WIDTH_MM 100.0 // max line width entered in a dialog
diff --git a/include/dialogs/panel_setup_netclasses.h b/include/dialogs/panel_setup_netclasses.h
index 174f6556ac..28e8e48a77 100644
--- a/include/dialogs/panel_setup_netclasses.h
+++ b/include/dialogs/panel_setup_netclasses.h
@@ -65,6 +65,7 @@ private:
void onUnitsChanged( wxCommandEvent& aEvent );
bool validateNetclassName( int aRow, const wxString& aName, bool focusFirst = true );
+ bool validateNetclassClearance( int aRow );
void rebuildNetclassDropdowns();
diff --git a/pcbnew/board_design_settings.cpp b/pcbnew/board_design_settings.cpp
index 5be62f14b0..768691fd36 100644
--- a/pcbnew/board_design_settings.cpp
+++ b/pcbnew/board_design_settings.cpp
@@ -1311,7 +1311,8 @@ int BOARD_DESIGN_SETTINGS::GetBiggestClearanceValue() const
biggest = std::max( biggest, constraint.Value().Min() );
}
- return biggest;
+ // Clip to avoid integer overflows in subsequent calculations
+ return std::min( biggest, MAXIMUM_CLEARANCE );
}
diff --git a/pcbnew/drc/drc_engine.cpp b/pcbnew/drc/drc_engine.cpp
index 35bc60db92..b0b38c5ff5 100644
--- a/pcbnew/drc/drc_engine.cpp
+++ b/pcbnew/drc/drc_engine.cpp
@@ -746,6 +746,24 @@ DRC_CONSTRAINT DRC_ENGINE::EvalRules( DRC_CONSTRAINT_T aConstraintType, const BO
if( c->constraint.m_Value.HasMax() )
constraint .m_Value.SetMax( c->constraint.m_Value.Max() );
+ switch( c->constraint.m_Type )
+ {
+ case CLEARANCE_CONSTRAINT:
+ case COURTYARD_CLEARANCE_CONSTRAINT:
+ case SILK_CLEARANCE_CONSTRAINT:
+ case HOLE_CLEARANCE_CONSTRAINT:
+ case EDGE_CLEARANCE_CONSTRAINT:
+ case PHYSICAL_CLEARANCE_CONSTRAINT:
+ case PHYSICAL_HOLE_CLEARANCE_CONSTRAINT:
+ if( constraint.m_Value.Min() > MAXIMUM_CLEARANCE )
+ constraint.m_Value.SetMin( MAXIMUM_CLEARANCE );
+
+ break;
+
+ default:
+ break;
+ }
+
// While the expectation would be to OR the disallow flags, we've already
// masked them down to aItem's type -- so we're really only looking for a
// boolean here.