[geos-commits] [SCM] GEOS branch 3.9 updated. 2d9cfd05594fdacc53a818f353f90c55b7b3ab9a

[git at osgeo.org]

This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GEOS".

The branch, 3.9 has been updated
       via  2d9cfd05594fdacc53a818f353f90c55b7b3ab9a (commit)
      from  0d3e09cc31101a7bd58051add7db0ce0aef6b6f5 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
commit 2d9cfd05594fdacc53a818f353f90c55b7b3ab9a
Author: Paul Ramsey <pramsey at cleverelephant.ca>
Date:   Wed Sep 8 14:24:29 2021 -0700

    Guard against some null inputs, references #1111

diff --git a/NEWS b/NEWS
index 46559bf..1e29d50 100644
--- a/NEWS
+++ b/NEWS
@@ -9,6 +9,7 @@ Changes in 3.9.2
   - Fix BufferOp to avoid artifacts in certain polygon buffers (#1101, Martin Davis)
   - Declare parameterless functions in geos_c.h as func(void) (Paul Ramsey)
   - Performance improvement in SegmentNodeList (Even Rouault)
+  - Catch memory leaks on createPolygon failure (#1111, Paul Ramsey)
 
 
 Changes in 3.9.1
diff --git a/capi/geos_ts_c.cpp b/capi/geos_ts_c.cpp
index 188d100..e43d6d8 100644
--- a/capi/geos_ts_c.cpp
+++ b/capi/geos_ts_c.cpp
@@ -2408,31 +2408,46 @@ extern "C" {
         //assert(0 != holes);
 
         return execute(extHandle, [&]() {
+            GEOSContextHandleInternal_t* handle = reinterpret_cast<GEOSContextHandleInternal_t*>(extHandle);
+            const GeometryFactory* gf = handle->geomFactory;
+            bool good_holes = true, good_shell = true;
 
-            std::vector<LinearRing*> tmpholes(nholes);
-            for (size_t i = 0; i < nholes; i++) {
-                LinearRing* lr = dynamic_cast<LinearRing*>(holes[i]);
-                if (! lr) {
-                    throw IllegalArgumentException("Hole is not a LinearRing");
+            // Validate input before taking ownership
+            for (std::size_t i = 0; i < nholes; i++) {
+                if ((!holes) || (!dynamic_cast<LinearRing*>(holes[i]))) {
+                    good_holes = false;
+                    break;
                 }
-                tmpholes[i] = lr;
             }
-            LinearRing* nshell = dynamic_cast<LinearRing*>(shell);
-            if(! nshell) {
-                throw IllegalArgumentException("Shell is not a LinearRing");
+            if (!dynamic_cast<LinearRing*>(shell)) {
+                good_shell = false;
             }
-            GEOSContextHandleInternal_t* handle = reinterpret_cast<GEOSContextHandleInternal_t*>(extHandle);
-            const GeometryFactory* gf = handle->geomFactory;
 
-            /* Create unique_ptr version for constructor */
-            std::vector<std::unique_ptr<LinearRing>> vholes;
-            vholes.reserve(nholes);
-            for (LinearRing* lr: tmpholes) {
-                vholes.emplace_back(lr);
+            // Contract for GEOSGeom_createPolygon is to take ownership of arguments
+            // which implies freeing them on exception,
+            // see https://trac.osgeo.org/geos/ticket/1111
+            if (!(good_holes && good_shell)) {
+                if (shell) delete shell;
+                for (std::size_t i = 0; i < nholes; i++) {
+                    if (holes && holes[i])
+                        delete holes[i];
+                }
+                if (!good_shell)
+                    throw IllegalArgumentException("Shell is not a LinearRing");
+                else
+                    throw IllegalArgumentException("Hole is not a LinearRing");
+            }
+
+            std::unique_ptr<LinearRing> tmpshell(static_cast<LinearRing*>(shell));
+            if (nholes) {
+                std::vector<std::unique_ptr<LinearRing>> tmpholes(nholes);
+                for (size_t i = 0; i < nholes; i++) {
+                    tmpholes[i].reset(static_cast<LinearRing*>(holes[i]));
+                }
+                return gf->createPolygon(std::move(tmpshell), std::move(tmpholes)).release();
             }
-            std::unique_ptr<LinearRing> shell(nshell);
+            return gf->createPolygon(std::move(tmpshell)).release();
 
-            return gf->createPolygon(std::move(shell), std::move(vholes)).release();
         });
     }
 
diff --git a/tests/unit/capi/GEOSGeom_createPolygonTest.cpp b/tests/unit/capi/GEOSGeom_createPolygonTest.cpp
index b560975..56a4746 100644
--- a/tests/unit/capi/GEOSGeom_createPolygonTest.cpp
+++ b/tests/unit/capi/GEOSGeom_createPolygonTest.cpp
@@ -50,5 +50,31 @@ void object::test<1>
     free(holes);
 }
 
+template<>
+template<>
+void object::test<2>
+()
+{
+    GEOSCoordSequence* shell_seq = GEOSCoordSeq_create(5, 2);
+
+    double shell_coords[] = {0,0, 0,10, 10,10, 10,0, 0,0};
+    for (unsigned int i = 0; i < 5; i++) {
+        GEOSCoordSeq_setXY(shell_seq, i, shell_coords[2*i], shell_coords[2*i+1]);
+    }
+
+    GEOSGeometry* shell = GEOSGeom_createLineString(shell_seq);
+    GEOSGeometry** holes = nullptr;
+    unsigned int nholes = 1;
+
+    // Returns null on exception, wrong input type for shell
+    GEOSGeometry* polygon = GEOSGeom_createPolygon(shell, holes, nholes);
+    ensure(polygon == nullptr);
+
+    // Shouldn't need this
+    if (polygon)
+        GEOSGeom_destroy(polygon);
+}
+
+
 } // namespace tut
 

-----------------------------------------------------------------------

Summary of changes:
 NEWS                                           |  1 +
 capi/geos_ts_c.cpp                             | 51 +++++++++++++++++---------
 tests/unit/capi/GEOSGeom_createPolygonTest.cpp | 26 +++++++++++++
 3 files changed, 60 insertions(+), 18 deletions(-)


hooks/post-receive
-- 
GEOS