kicad-developers team mailing list archive

[Andreas Buhr <andreas@xxxxxxxxxxxxxx>]

Dear Kicad developers,

the attached patch fixes two bugs in SHAPE_POLY_SET.

There were two problems in the triangulation caching
of SHAPE_POLY_SET:
First there was a double free:
While SHAPE_POLY_SET implements the copy constructor,
it did not implement the operator=, which resulted
in the default operator= being generated by the
compiler. The default operator= copied the member
m_triangulatedPolys, which is a std::vector of pointers.
So after operator= execution, there are two SHAPE_POLY_SET
having pointers to the same TRIANGULATED_POLYGONs, each
of them deleting them in their destructors. This led
to segfaults, because calling
TransformShapeWithClearanceToPolygon on a Zone
uses operator= to copy the contained SHAPE_POLY_SET.
The new SHAPE_POLY_SET then went out of scope and
deleted the cached triangulation within the Zone.

This first problem is fixed by implementing operator=
for SHAPE_POLY_SET.

Second, there was a memory leak: Calling
"CacheTriangulation" on a SHAPE_POLY_SET,
then changing the polygon and then calling
"CacheTriangulation" again led to
leaking the
triangulations generated in the first call.

This second problem is fixed by holding
the cached triangulations in a unique_ptr.

I hope you find this patch useful.

Cheers,
Andreas

>From 947bf92c5a83cbd7202a6ad2220ac8c15aa3f9c8 Mon Sep 17 00:00:00 2001
From: Andreas Buhr <andreas@xxxxxxxxxxxxxx>
Date: Fri, 8 Dec 2017 12:20:02 +0100
Subject: [PATCH] fix double free and memory leak in SHAPE_POLY_SET

There were two problems in the triangulation caching
of SHAPE_POLY_SET:
First there was a double free:
While SHAPE_POLY_SET implements the copy constructor,
it did not implement the operator=, which resulted
in the default operator= being generated by the
compiler. The default operator= copied the member
m_triangulatedPolys, which is a std::vector of pointers.
So after operator= execution, there are two SHAPE_POLY_SET
having pointers to the same TRIANGULATED_POLYGONs, each
of them deleting them in their destructors. This led
to segfaults, because calling
TransformShapeWithClearanceToPolygon on a Zone
uses operator= to copy the contained SHAPE_POLY_SET.
The new SHAPE_POLY_SET then went out of scope and
deleted the cached triangulation within the Zone.

This first problem is fixed by implementing operator=
for SHAPE_POLY_SET.

Second, there was a memory leak: Calling
"CacheTriangulation" on a SHAPE_POLY_SET,
then changing the polygon and then calling
"CacheTriangulation" again led to
leaking the
triangulations generated in the first call.

This second problem is fixed by holding
the cached triangulations in a unique_ptr.
---
 common/geometry/shape_poly_set.cpp | 27 +++++++++++++++------------
 include/geometry/shape_poly_set.h  |  9 +++++----
 2 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/common/geometry/shape_poly_set.cpp b/common/geometry/shape_poly_set.cpp
index ff2ad87..3b81809 100644
--- a/common/geometry/shape_poly_set.cpp
+++ b/common/geometry/shape_poly_set.cpp
@@ -33,7 +33,6 @@
 #include <list>
 #include <algorithm>
 #include <unordered_set>
-#include <memory>
 
 #include <common.h>
 #include <md5_hash.h>
@@ -58,14 +57,6 @@ SHAPE_POLY_SET::SHAPE_POLY_SET( const SHAPE_POLY_SET& aOther ) :
 {
 }
 
-
-SHAPE_POLY_SET::~SHAPE_POLY_SET()
-{
-    for( auto p : m_triangulatedPolys )
-        delete p;
-}
-
-
 SHAPE* SHAPE_POLY_SET::Clone() const
 {
     return new SHAPE_POLY_SET( *this );
@@ -1871,6 +1862,19 @@ SHAPE_POLY_SET::POLYGON SHAPE_POLY_SET::chamferFilletPolygon( CORNER_MODE aMode,
 }
 
 
+SHAPE_POLY_SET &SHAPE_POLY_SET::operator=(const SHAPE_POLY_SET & aOther)
+{
+    static_cast<SHAPE&>(*this) = aOther;
+    m_polys = aOther.m_polys;
+
+    // reset poly cache:
+    m_hash = MD5_HASH{};
+    m_triangulationValid = false;
+    m_triangulatedPolys.clear();
+    return *this;
+}
+
+
 typedef std::map<p2t::Point*, int>  P2T_MAP;
 typedef std::vector<p2t::Point*>    P2T_VEC;
 
@@ -2022,9 +2026,8 @@ void SHAPE_POLY_SET::CacheTriangulation()
 
     for( int i = 0; i < tmpSet.OutlineCount(); i++ )
     {
-        auto p = new TRIANGULATED_POLYGON();
-        m_triangulatedPolys.push_back( p );
-        triangulateSingle( tmpSet.Polygon( i ), *p );
+        m_triangulatedPolys.push_back( std::make_unique<TRIANGULATED_POLYGON>() );
+        triangulateSingle( tmpSet.Polygon( i ), *m_triangulatedPolys.back() );
     }
 
     m_triangulationValid = true;
diff --git a/include/geometry/shape_poly_set.h b/include/geometry/shape_poly_set.h
index cec3491..78308bc 100644
--- a/include/geometry/shape_poly_set.h
+++ b/include/geometry/shape_poly_set.h
@@ -28,6 +28,7 @@
 
 #include <vector>
 #include <cstdio>
+#include <memory>
 #include <geometry/shape.h>
 #include <geometry/shape_line_chain.h>
 
@@ -406,8 +407,6 @@ class SHAPE_POLY_SET : public SHAPE
          */
         SHAPE_POLY_SET( const SHAPE_POLY_SET& aOther );
 
-        ~SHAPE_POLY_SET();
-
         /**
          * Function GetRelativeIndices
          *
@@ -585,7 +584,7 @@ class SHAPE_POLY_SET : public SHAPE
 
         const TRIANGULATED_POLYGON* TriangulatedPolygon( int aIndex ) const
         {
-            return m_triangulatedPolys[aIndex];
+            return m_triangulatedPolys[aIndex].get();
         }
 
 
@@ -1120,6 +1119,8 @@ class SHAPE_POLY_SET : public SHAPE
 
     public:
 
+        SHAPE_POLY_SET& operator=(const SHAPE_POLY_SET&);
+
         void CacheTriangulation();
         bool IsTriangulationUpToDate() const;
 
@@ -1128,7 +1129,7 @@ class SHAPE_POLY_SET : public SHAPE
 
         MD5_HASH checksum() const;
 
-        std::vector<TRIANGULATED_POLYGON*> m_triangulatedPolys;
+        std::vector<std::unique_ptr<TRIANGULATED_POLYGON>> m_triangulatedPolys;
         bool m_triangulationValid = false;
         MD5_HASH m_hash;
 
-- 
2.7.4