Ticket #227 (closed defect: fixed)

Opened 3 years ago

Last modified 3 years ago

Deleting 'void*' is undefined in IntervalRTreeLeafNode class.

Reported by: mloskot Owned by: strk
Priority: major Milestone: 3.2.0
Component: Core Version: svn-trunk
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

Changed 3 years ago by mloskot

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)

Changed 3 years ago by pramsey

  • milestone set to 3.2.0

Changed 3 years ago by pramsey

  • owner changed from geos-devel@… to strk

Changed 3 years ago by strk

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 :)

Changed 3 years ago by strk

  • status changed from new to closed
  • resolution set to fixed

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.