Opened 14 years ago

Closed 13 years ago

#227 closed defect (fixed)

Deleting 'void*' is undefined in IntervalRTreeLeafNode class.

Reported by: mloskot Owned by: strk
Priority: major Milestone: 3.2.0
Component: Core Version: main
Severity: Significant Keywords: void
Cc:

Description

According to section 5.3.5/3 of C++ Standard, there is undefined behavior in IntervalRTreeLeafNode destructor (line 50) because item (line 39) is a pointer of type void.

Change History (5)

comment:1 by mloskot, 14 years ago

A little irc discussion with Sandro about this issue from Jan 30, 2009:

<strk> deleting void* ?
<mloskot> in general, it's trivial to fix - tricky part ->  as long as we know base type of the item
<strk> we never know
<mloskot> why?
<strk> you'll have to define a base class just for that
<strk> and have the actual items subclass it
<strk> by either multiple-inheritance or wrapping
<mloskot> what about inserting through a wrapper instead of plain void*?
<strk> what I said, a wrapper (an abstract base)
<mloskot> I'd need to think, but for now simplest is to add a thin empty base class.
<strk> yeah, I'd go there
<mloskot> perhaps we could have a common base class template to all geos objects and they we could specialize it with some traits
<mloskot> like flags
<strk> I'd rather find "local" solutions
<strk> or you could end up walking on a looong road, and full of perils
<mloskot> like
<mloskot> class polygon : public geos::traits<geos::indexable>
<mloskot> similar to boost.operators
<mloskot> later on
<mloskot> class envelope : public geos::traits<geos::indexable, geos::addable>
<mloskot> etc.
<strk> eh
<strk> and bringing all contributors up in the learning curve
<strk> to even *read* that :)
<strk> there aren't many void pointers around atm right?
<mloskot> you mean reading the templatized thing?
<strk> it would change the semantic of everything, won't it?
<strk> or, maybe not
<strk> but the user can't just rely on those added fields to be available can it?
<strk> or does the traits trick ensure a given layout in the virtual table ?
<mloskot> it's only or as many as change of type(s)
<mloskot> anyway, i don't like void* item ;-)
<strk> me neither
<strk> as I said, I'd take an abstract base pointer
<strk> and move the problem elsewhere 
<strk> local-base class
<strk> you'd then adapt, wrap or inherit for use
<mloskot> we have everything polymorphic anyway, so one more abstract type wouldn't change much for the performance, or complexity
<strk> right
<strk> I think I used thi trick elsewhere in geos codebase (not sure though)

comment:2 by pramsey, 13 years ago

Milestone: 3.2.0

comment:3 by pramsey, 13 years ago

Owner: changed from geos-devel@… to strk

comment:4 by strk, 13 years ago

I checked. That class is unused by core GEOS and headers are not installed. It's a teoreticall bug, but I'd rather drop the whole class from GEOS before it's too late :)

comment:5 by strk, 13 years ago

Resolution: fixed
Status: newclosed

Alright, I reworked ownership of those objects and tracked all users (I did find one: algorithm::locate::IndexedPointInAreaLocator) to properly avoid memory leaks.

Note: See TracTickets for help on using tickets.