From 1c459e9a67151a6f028ac3a108b338e850126bfb Mon Sep 17 00:00:00 2001
From: StefanBruens <stefan.bruens@rwth-aachen.de>
Date: Wed, 13 Mar 2024 21:43:12 +0000
Subject: [PATCH] Fix triangulationValid check race for zone fill

The m_triangulationValid flag is used in several places without holding
the mutex, thus it should only ever be set when the triangulation is
guaranteed to be valid.

This can either be done by protecting both data and flag by the same
mutex, or updating the flag only after the triangulation has finished.

Also fix the case when the triangulation actually fails, the flag should
not be set in this case.

While at it, simplify the recalculation check. Only if both the
triangulation is valid, and the data hash is unchanged the recalculation
can be skipped - this is typically the case when two threads try to
update the cache concurrently, the second one will block at the mutex,
and will see the valid data after the first thread has finished.

Fixes #17180
---
 libs/kimath/include/geometry/shape_poly_set.h |  3 +-
 libs/kimath/src/geometry/shape_poly_set.cpp   | 38 +++++++++----------
 2 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/libs/kimath/include/geometry/shape_poly_set.h b/libs/kimath/include/geometry/shape_poly_set.h
index ac5b2325744..e3d1ac18f35 100644
--- a/libs/kimath/include/geometry/shape_poly_set.h
+++ b/libs/kimath/include/geometry/shape_poly_set.h
@@ -28,6 +28,7 @@
 #ifndef __SHAPE_POLY_SET_H
 #define __SHAPE_POLY_SET_H
 
+#include <atomic>
 #include <cstdio>
 #include <deque>                        // for deque
 #include <iosfwd>                       // for string, stringstream
@@ -1538,7 +1539,7 @@ protected:
     std::vector<POLYGON>                               m_polys;
     std::vector<std::unique_ptr<TRIANGULATED_POLYGON>> m_triangulatedPolys;
 
-    bool        m_triangulationValid = false;
+    std::atomic<bool> m_triangulationValid = false;
     std::mutex  m_triangulationMutex;
 
 private:
diff --git a/libs/kimath/src/geometry/shape_poly_set.cpp b/libs/kimath/src/geometry/shape_poly_set.cpp
index dacc1e8a07c..da379ad309a 100644
--- a/libs/kimath/src/geometry/shape_poly_set.cpp
+++ b/libs/kimath/src/geometry/shape_poly_set.cpp
@@ -2883,7 +2883,7 @@ SHAPE_POLY_SET &SHAPE_POLY_SET::operator=( const SHAPE_POLY_SET& aOther )
     }
 
     m_hash = aOther.m_hash;
-    m_triangulationValid = aOther.m_triangulationValid;
+    m_triangulationValid = aOther.m_triangulationValid.load();
 
     return *this;
 }
@@ -2985,25 +2985,16 @@ void SHAPE_POLY_SET::cacheTriangulation( bool aPartition, bool aSimplify,
                                          std::vector<std::unique_ptr<TRIANGULATED_POLYGON>>* aHintData )
 {
     std::unique_lock<std::mutex> lock( m_triangulationMutex );
-    bool recalculate = !m_hash.IsValid();
-    MD5_HASH hash;
-
-    if( !m_triangulationValid )
-        recalculate = true;
 
-    if( !recalculate )
+    if( m_triangulationValid && m_hash.IsValid() )
     {
-        hash = checksum();
-
-        if( m_hash != hash )
-        {
-            m_hash = hash;
-            recalculate = true;
-        }
+        if( m_hash == checksum() )
+            return;
     }
 
-    if( !recalculate )
-        return;
+    // Invalidate, in case anything goes wrong below
+    m_triangulationValid = false;
+    m_hash.SetValid( false );
 
     auto triangulate =
             []( SHAPE_POLY_SET& polySet, int forOutline,
@@ -3054,7 +3045,6 @@ void SHAPE_POLY_SET::cacheTriangulation( bool aPartition, bool aSimplify,
             };
 
     m_triangulatedPolys.clear();
-    m_triangulationValid = true;
 
     if( aPartition )
     {
@@ -3081,6 +3071,12 @@ void SHAPE_POLY_SET::cacheTriangulation( bool aPartition, bool aSimplify,
             {
                 wxLogTrace( TRIANGULATE_TRACE, "Failed to triangulate partitioned polygon %d", ii );
             }
+            else
+            {
+                m_hash = checksum();
+                // Set valid flag only after everything has been updated
+                m_triangulationValid = true;
+            }
         }
     }
     else
@@ -3095,9 +3091,13 @@ void SHAPE_POLY_SET::cacheTriangulation( bool aPartition, bool aSimplify,
         {
             wxLogTrace( TRIANGULATE_TRACE, "Failed to triangulate polygon" );
         }
+        else
+        {
+            m_hash = checksum();
+            // Set valid flag only after everything has been updated
+            m_triangulationValid = true;
+        }
     }
-
-    m_hash = checksum();
 }
 
 
-- 
GitLab