Fabmaster: never add null holes to polygons

By adding points to holes as we go, we leave ourselves open
to adding invalid (<3 point) holes to the polygons. This causes
crashes if it happens, so we should never allow it to happen.

Instead of building holes in-place, construct them externally
and only add them if they are valid.

This probably wouldn't actually happen for a valid Fabmaster
file, but if it did, it would be bad new, and we shouldn't leave
crashable pathways facing user input.

libs/kimath/include/geometry/shape_utils.h

@@ -46,6 +46,7 @@ class HALF_LINE;
 class LINE;
 class SEG;
 class SHAPE_RECT;
+class SHAPE_POLY_SET;
 struct TYPED_POINT2I;
 
 namespace KIGEOM
@@ -162,4 +163,19 @@ std::vector<TYPED_POINT2I> GetCircleKeyPoints( const CIRCLE& aCircle, bool aIncl
  */
 SHAPE_LINE_CHAIN RectifyPolygon( const SHAPE_LINE_CHAIN& aPoly );
 
+
+/**
+ * Adds a hole to a polygon if it is valid (i.e. it has 3 or more points
+ * and a non-zero area.)
+ *
+ * This is important if you are reading in a polygon from a file and
+ * aren't sure if it will end up being valid. Leaving invalid holes in
+ * a SHAP_POLY_SET will violate the invariant that holes are non-null.
+ *
+ * @param aOutline is the outline to add the hole to.
+ * @param aHole is the hole to add.
+ * @return true if the hole was added, false if it was not.
+ */
+bool AddHoleIfValid( SHAPE_POLY_SET& aOutline, SHAPE_LINE_CHAIN&& aHole );
+
 } // namespace KIGEOM

libs/kimath/src/geometry/shape_utils.cpp

@@ -28,6 +28,7 @@
 #include <geometry/half_line.h>
 #include <geometry/line.h>
 #include <geometry/point_types.h>
+#include <geometry/shape_poly_set.h>
 #include <geometry/shape_rect.h>
 
 
@@ -375,4 +376,16 @@ SHAPE_LINE_CHAIN KIGEOM::RectifyPolygon( const SHAPE_LINE_CHAIN& aPoly )
     raOutline.Simplify();
 
     return raOutline;
-}
+}
+
+
+bool KIGEOM::AddHoleIfValid( SHAPE_POLY_SET& aOutline, SHAPE_LINE_CHAIN&& aHole )
+{
+    if( aHole.PointCount() < 3 || aHole.Area() == 0 )
+    {
+        return false;
+    }
+
+    aOutline.AddHole( std::move( aHole ) );
+    return true;
+}

pcbnew/pcb_io/fabmaster/import_fabmaster.cpp

@@ -50,9 +50,11 @@
 #include <zone.h>
 #include <common.h>
 #include <geometry/shape_arc.h>
+#include <geometry/shape_utils.h>
 #include <string_utils.h>
 #include <progress_reporter.h>
 #include <math/util.h>
+
 #include <wx/filename.h>
 
 
@@ -2790,8 +2792,6 @@ bool FABMASTER::loadZone( BOARD* aBoard, const std::unique_ptr<FABMASTER::TRACE>
     if( aLine->segment.size() < 3 )
         return false;
 
-    int last_subseq = 0;
-    int hole_idx = -1;
     SHAPE_POLY_SET* zone_outline = nullptr;
     ZONE* zone = nullptr;
 
@@ -2841,7 +2841,29 @@ bool FABMASTER::loadZone( BOARD* aBoard, const std::unique_ptr<FABMASTER::TRACE>
 
     zone_outline->NewOutline();
 
+    std::unique_ptr<SHAPE_LINE_CHAIN> pending_hole = nullptr;
+    SHAPE_LINE_CHAIN*                 active_chain = &zone_outline->Outline( 0 );
 
+    const auto add_hole_if_valid = [&]()
+    {
+        if( pending_hole )
+        {
+            pending_hole->SetClosed( true );
+
+            // If we get junk holes, assert, but don't add them to the zone, as that
+            // will cause crashes later.
+            if( !KIGEOM::AddHoleIfValid( *zone_outline, std::move( *pending_hole ) ) )
+            {
+                wxLogMessage( _( "Invalid hole with %d points in zone on layer %s with net %s" ),
+                              pending_hole->PointCount(), zone->GetLayerName(),
+                              zone->GetNetname() );
+            }
+
+            pending_hole.reset();
+        }
+    };
+
+    int last_subseq = 0;
     for( const auto& seg : aLine->segment )
     {
         if( seg->subseq > 0 && seg->subseq != last_subseq )
@@ -2851,33 +2873,52 @@ bool FABMASTER::loadZone( BOARD* aBoard, const std::unique_ptr<FABMASTER::TRACE>
             if( aLine->lclass == "BOUNDARY" )
                 break;
 
-            hole_idx = zone_outline->AddHole( SHAPE_LINE_CHAIN{} );
-            last_subseq = seg->subseq;
+            add_hole_if_valid();
+            pending_hole = std::make_unique<SHAPE_LINE_CHAIN>();
+            active_chain = pending_hole.get();
             last_subseq = seg->subseq;
         }
 
         if( seg->shape == GR_SHAPE_LINE )
         {
             const GRAPHIC_LINE* src = static_cast<const GRAPHIC_LINE*>( seg.get() );
+            const VECTOR2I      start( src->start_x, src->start_y );
+            const VECTOR2I      end( src->end_x, src->end_y );
 
-            if( zone_outline->VertexCount( 0, hole_idx ) == 0 )
-                zone_outline->Append( src->start_x, src->start_y, 0, hole_idx );
+            if( active_chain->PointCount() == 0 )
+            {
+                active_chain->Append( start );
+            }
+            else
+            {
+                const VECTOR2I& last = active_chain->CPoint( -1 );
 
-            zone_outline->Append( src->end_x, src->end_y, 0, hole_idx );
+                // Not if this can ever happen, or what do if it does (add both points?).
+                if( last != start )
+                {
+                    wxLogError( _( "Outline seems discontinuous: last point was %s, "
+                                   "start point of next segment is %s" ),
+                                last.Format(), start.Format() );
+                }
+            }
+
+            active_chain->Append( end );
         }
         else if( seg->shape == GR_SHAPE_ARC || seg->shape == GR_SHAPE_CIRCLE )
         {
             /* Even if it says "circle", it's actually an arc, it's just closed */
             const GRAPHIC_ARC* src = static_cast<const GRAPHIC_ARC*>( seg.get() );
-            zone_outline->Hole( 0, hole_idx ).Append( src->result );
+            active_chain->Append( src->result );
         }
         else
         {
-            wxASSERT_MSG( false,
-                          wxString::Format( "Invalid shape type %d in zone outline", seg->shape ) );
+            wxLogError( _( "Invalid shape type %d in zone outline" ), seg->shape );
         }
     }
 
+    // Finalise the last hole, if any
+    add_hole_if_valid();
+
     if( zone_outline->Outline( 0 ).PointCount() >= 3 )
     {
         zone->SetOutline( zone_outline );