Opened 18 years ago
Closed 15 years ago
#86 closed defect (fixed)
Possible memory leaks caused by LineString constructor
Reported by: | mloskot | Owned by: | strk |
---|---|---|---|
Priority: | critical | Milestone: | 3.2.0 |
Component: | Core | Version: | 2.2.3 |
Severity: | Significant | Keywords: | imported, phpbugtracker |
Cc: |
Description (last modified by )
There is a high possibility that constructor of LineString class will cause memory leaks.
Here is small snipped with current version of this ctor:
LineString::LineString(CoordinateSequence* newCoords, const GeometryFactory *factory) : Geometry(factory) { if (newCoords==NULL) { points=factory->getCoordinateSequenceFactory()->create(NULL); return; } if (newCoords->getSize()==1) { throw util::IllegalArgumentException("point array must contain 0 or >1 elementsn"); } // 'points' WILL LEAK WHEN EXCEPTION ABOVE IS THROWN points=newCoords; // <--- 1 }
Now, the small piece of code with how I create LineString object in one of my test. This test tries to create one-point linestring intentionaly.
void test() { // Single-element sequence of coordiantes CoordinateArraySequence* pseq = new CoordinateArraySequence(); pseq->add(geos::geom::Coordinate(0, 0, 0)); try { // Create incomplete linstring // 'ls' IS ASSUMED TO TAKE OWNERSHIP OF 'pseq' SEQUENCE geos::geom::LineString ls(pseq, &factory_); // <--- [2] // WE EXPECT THE EXCEPTION } catch (geos::util::IllegalArgumentException const& e) { // [3] <--- BUM! BUM! BUM! // [4] <--- REQUIRED TO AVOID BUM! delete pseq; std::cout << e.what() << std::endl; } }
Now, the explanation:
- New coordiante sequence is created with only one point
- New linestringis created with one-point sequence that is expected to be invalid
- LineString ctor recognizes this sequence as invalid and throws IllegalArgumentException
- At this moment, the line 1 is not called, so the sequence previously created does not become a part of LineString object and LineString::points member is left as null
- When exception is thrown in line 3, the LineString destructor is called but 'points' is null, so the sequence is not deallocated (BUM!)
- Finally, sequnce 'pseq' leaks
- To avoid the memory leaks, temporary solution is to deallocate it on the client side, when exception is catched, in line 4.
The main problem is that LineString ctor does not promises (prepares LineString object well) that all resources LineString is responsible for will be deallocated by dctor in case of problems.
The simplest possible solution is to move line 1: points=newCoords; <--- 1
to the beginning of the LineString::LineString(). This won't be a problem if newCoords is null pointer, because further checks will do the job well. If newCoords is not a null pointer, then the sequence will be assigned to points member and in case of exception, LineString destructor will deallocated it properly.
Change History (31)
comment:2 by , 18 years ago
Resolution: | fixed → deferred |
---|
Unfortunately my simple solution does not solve the problem. LineString will still leak when coordinates sequence is of size = 1. This is because when ctor throws exception, destructor is never called, so 'points' sequence is not deallocated. I'm preparing some deeper explanation with solution proposal. I'll send it on the mailing list first, subject "Exception-safety". So, please, refer to this post. I changed status of this Bug Report to Reopened + Deferred.
comment:3 by , 17 years ago
Component: | Core → SWIG |
---|---|
Resolution: | deferred → not a bug |
Severity: | Significant → Critical |
Version: | 3.0.0 → 2.2.1 |
comment:4 by , 17 years ago
Component: | SWIG → loader/dumper |
---|---|
Resolution: | not a bug → duplicate |
Version: | 2.2.1 → 1.1.1 |
comment:5 by , 17 years ago
Severity: | Critical → Unassigned |
---|
comment:6 by , 17 years ago
Component: | loader/dumper → build scripts |
---|---|
Resolution: | duplicate → none |
Version: | 1.1.1 → 1.1.7 |
comment:7 by , 17 years ago
Component: | build scripts → SWIG |
---|---|
Resolution: | none → not a bug |
Severity: | Unassigned → Significant |
Version: | 1.1.7 → 3.0.0 |
comment:8 by , 17 years ago
Component: | SWIG → Core |
---|---|
Resolution: | not a bug → duplicate |
Severity: | Significant → Content |
Version: | 3.0.0 → 1.0.3 |
comment:9 by , 17 years ago
I cleaned this bug from some spam. Looks like we're experiencing frequent spam attacks.
comment:10 by , 17 years ago
Resolution: | duplicate → not a bug |
---|---|
Version: | 1.0.3 → 1.1.6 |
comment:11 by , 17 years ago
Component: | Core → loader/dumper |
---|---|
Resolution: | not a bug → won't fix |
Version: | 1.1.6 → 1.1.2 |
comment:12 by , 17 years ago
Component: | loader/dumper → Core |
---|---|
Resolution: | won't fix → not a bug |
Severity: | Content → Feature Request |
Version: | 1.1.2 → 1.0.5 |
comment:13 by , 17 years ago
Resolution: | not a bug → duplicate |
---|---|
Severity: | Feature Request → Significant |
Version: | 1.0.5 → 1.1.4 |
comment:14 by , 17 years ago
Component: | Core → loader/dumper |
---|
comment:15 by , 17 years ago
Severity: | Significant → Critical |
---|---|
Version: | 1.1.4 → 1.0.2 |
comment:16 by , 17 years ago
Component: | loader/dumper → Core |
---|---|
Resolution: | duplicate → not a bug |
Severity: | Critical → Idea |
Version: | 1.0.2 → 1.2.1 |
comment:17 by , 17 years ago
Resolution: | not a bug → fixed |
---|---|
Severity: | Idea → Significant |
Version: | 1.2.1 → 1.1.7 |
comment:18 by , 17 years ago
Resolution: | fixed → works for me |
---|---|
Severity: | Significant → Annoyance |
Version: | 1.1.7 → head |
comment:19 by , 17 years ago
Component: | Core → build scripts |
---|---|
Resolution: | works for me → none |
Severity: | Annoyance → Unassigned |
Version: | head → 1.2.0 |
comment:20 by , 17 years ago
Component: | build scripts → Build scripts |
---|---|
Version: | 1.2.0 → 2.2.3 |
Removed spam from the comment.
comment:37 by , 16 years ago
Description: | modified (diff) |
---|---|
Reporter: | changed from | to
comment:38 by , 16 years ago
Description: | modified (diff) |
---|
comment:39 by , 16 years ago
Milestone: | imported |
---|
comment:40 by , 16 years ago
Component: | Build scripts → Core |
---|---|
Priority: | 1 → critical |
Severity: | Unassigned → Significant |
comment:41 by , 15 years ago
Milestone: | → 3.2.0 |
---|
comment:43 by , 15 years ago
Status: | new → assigned |
---|
Ever been ? Mat: do you have a testcase exploiting the leak ?
comment:44 by , 15 years ago
I believe there is no need for a test case. The code I outline quoted clearly shows there is lack of use of RAII.
comment:45 by , 15 years ago
I'm asking because I did try to create a LineString with a CoordinateSequece of size 1 and valgrind didn't give me the CoordinateSequence as leaking. Instead it gave me some std::string leaks which seems unrelated (the message passed to the exception thrown)
comment:47 by , 15 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Well, I couldn't handle to reproduce and so I'll take for fixed.