Possible memory leak in WKTReader with partial MULTIPOLYGON

[goatbar]

Seeing this with llvm fuzzing with ASAN and geos 3.5.0.

Am I doing something wrong with the API or is this a bug with WKTReader not cleaning up in the case of an exception?

Given this input string:

MULTIPOLYGON(

And this code:

// Copyright 2017 Google Inc. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//     http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#include <stddef.h>
#include <stdint.h>
#include <string>

#include "third_party/geos/include/geos/geom/CoordinateSequence.h"
#include "third_party/geos/include/geos/geom/Geometry.h"
#include "third_party/geos/include/geos/io/WKTReader.h"

typedef std::unique_ptr<geos::geom::Geometry> GeomPtr;
typedef std::unique_ptr<geos::geom::CoordinateSequence> CoordSeqPtr;

extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
  // Ensure there is a NUL at the end of the data passed to the reader.
  const std::string s(reinterpret_cast<const char *>(data), size);

  try {
    geos::io::WKTReader reader;
    GeomPtr geom(reader.read(s));
    CoordSeqPtr coords(geom->getCoordinates());
    coords->getDimension();
  } catch (...) {
    // NOP
  }

  return 0;
}

Running: leak-1c9d82726612fdd23af7740675fa675af0cebe0f

=================================================================
==23962==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0x500942 in operator new(unsigned long) third_party/llvm/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:82:35
    #1 0x66cffc in geos::io::WKTReader::readMultiPolygonText(geos::io::StringTokenizer*) third_party/geos/geos-3.5.0/src/io/WKTReader.cpp:422:31
    #2 0x668ced in geos::io::WKTReader::readGeometryTaggedText(geos::io::StringTokenizer*) third_party/geos/geos-3.5.0/src/io/WKTReader.cpp:240:10
    #3 0x668784 in geos::io::WKTReader::read(std::string const&) third_party/geos/geos-3.5.0/src/io/WKTReader.cpp:67:4
    #4 0x5037a3 in LLVMFuzzerTestOneInput third_party/geos/tests/io/wktreader_fuzzer.cc:33:23

SUMMARY: AddressSanitizer: 24 byte(s) leaked in 1 allocation(s).

[strk]

Was this fixed by [f825b41004b37fcb0dd4ea76f22b5b6aa447b2ac/git] ?

[goatbar]

Sadly, no, ​https://github.com/OSGeo/geos/pull/83 does not fix the issue.

source:trunk/src/io/WKTReader.cpp@4052#L422​

`new vector' is leaked if there is a throw before return. I am not sure what shape the fix should be. If this were C++11, putting it in a unique_ptr would make sure it was deleted as the stack unwound. A closer class with a delete in the destructor would work too. Or simplest might be to try / catch the exception. I'm not very experienced with exceptions, so I haven't tried catch the exception yet.

MultiPolygon* WKTReader::readMultiPolygonText(StringTokenizer *tokenizer) {
        string nextToken=getNextEmptyOrOpener(tokenizer);
        if (nextToken=="EMPTY") {
                return geometryFactory->createMultiPolygon(NULL);
        }
        vector<Geometry *> *polygons=new vector<Geometry *>();  // <--- This is leaked.
        Polygon *polygon=readPolygonText(tokenizer);
        polygons->push_back(polygon);
        nextToken=getNextCloserOrComma(tokenizer);
        while(nextToken==",") {
                Polygon *polygon=readPolygonText(tokenizer);
                polygons->push_back(polygon);
                nextToken=getNextCloserOrComma(tokenizer);
        }
        MultiPolygon *ret = geometryFactory->createMultiPolygon(polygons);
        //for (int i=0; i<polygons->size(); i++) delete (*polygons)[i];
        //delete polygons;
        return ret;
}

I would guess that every other read method with a new will also leak.

[mloskot]

Kurt, this is classic lack of RAII in GEOS. Switch to C++11 should help, replacing auto_ptr with unique_ptr and using unique_ptr in as many places as possible.

In this particular case, vector<Geometry *> *polygons=new vector<Geometry *>( could be replaced with intermediate vector on stack, once fully created, allocate new on heap and move polygons data into it before passing to createMultiPolygon.

This however is partial fix: it will avoid leaking polygons = new vector<Geometry *> but won't avoid leaking heap-allocated items of polygons.

Long term solution could be switching away from heap-allocated vectors to use of rvalues with move semantic.

[goatbar]

Working on a proposed simple patch of this form. I'll have it ready in the next day.

        Polygon *polygon = NULL;
        try {
                polygon=readPolygonText(tokenizer);
        } catch (ParseException const& e) {
                delete polygons;
                throw ParseException(e.what());
        }

This happens a number of locations, e.g. multilinestring too.

mloskot: Agreed, the whole move semantics world will make this a lot simple in the long run.

[goatbar]

My proposed solution is in ​https://github.com/OSGeo/geos/pull/84

It's similar to other methods in the class. Catching ... is especially important as not all exceptions are geos::io::ParseException. e.g. I got a geos::util::IllegalArgumentException for a malformed LinearRing inside a GeometryCollection.

[Sandro Santilli <strk@…>]

In 8447747/git:

Fix leaks in WKT parser

Cleanup if failing to parse the WKT MultiLineString, MultiPolygon,
and GeometryCollection string.

Wrap parsing in try blocks.
Catch any exception, cleanup, and rethrow.

Closes #830
Includes testcase

Signed-off-by: Sandro Santilli <strk@…>

[Sandro Santilli <strk@…>]

In a823bca/git:

Fix leaks in WKT parser

Cleanup if failing to parse the WKT MultiLineString, MultiPolygon,
and GeometryCollection string.

Wrap parsing in try blocks.
Catch any exception, cleanup, and rethrow.

Closes #830
Includes testcase

Signed-off-by: Sandro Santilli <strk@…>