Re: [geos-devel] GEOS cross-heap problems
On Wed, May 13, 2009 at 12:19:23AM -0400, Frank Warmerdam wrote: Paul Ramsey wrote: I'd like to hear from others on this. OK - though I don't see the downside. Downstream packages only need to check for, and use GEOSFree() if they want. We are just adding a new option, not altering an existing one or removing anything. Agreed, no compatibility problem adding a GEOSFree. Only, it'll have to stay (I've read Mateusz suggesting using type-specific destroyers). Worth going throught the CAPI header files and properly document when to use GEOSFree on the return right above the function. PS: I've noticed Frank added a ChangeLog entry manually. Was that the way ? As I haven't touched it :( --strk; Free GIS Flash consultant/developer () ASCII Ribbon Campaign http://foo.keybit.net/~strk/services.html /\ Keep it simple! ___ geos-devel mailing list geos-devel@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/geos-devel
Re: [geos-devel] GEOS cross-heap problems
strk wrote: On Wed, May 13, 2009 at 12:19:23AM -0400, Frank Warmerdam wrote: Paul Ramsey wrote: I'd like to hear from others on this. OK - though I don't see the downside. Downstream packages only need to check for, and use GEOSFree() if they want. We are just adding a new option, not altering an existing one or removing anything. Agreed, no compatibility problem adding a GEOSFree. Folks, OK, I've back ported the GEOSFree() function into the 3.1 branch. Note, it will be very important to ensure that the C API version is upgraded for the 3.1.1 release. I'm not sure how we remember to do stuff like this. I see there have been a few fixes in the 3.1 branch: http://trac.osgeo.org/geos/log/branches/3.1 Perhaps it would be appropriate for us to roll out a 3.1.1? Only, it'll have to stay (I've read Mateusz suggesting using type-specific destroyers). I cannot imagine why a type specific destroyer would be needed for simple malloc()/free() buffer allocations. Worth going throught the CAPI header files and properly document when to use GEOSFree on the return right above the function. I have slightly modified the geos_c.h.in to reflect use of GEOSFree() but I wonder if there is other documentation of the C API. PS: I've noticed Frank added a ChangeLog entry manually. Was that the way ? As I haven't touched it :( I assumed this was expected practice since the ChangeLog exists and is updated by at least some developers. Best regards, -- ---+-- I set the clouds in motion - turn up | Frank Warmerdam, warmer...@pobox.com light and sound - activate the windows | http://pobox.com/~warmerdam and watch the world go round - Rush| Geospatial Programmer for Rent ___ geos-devel mailing list geos-devel@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/geos-devel
Re: [geos-devel] GEOS cross-heap problems
Frank Warmerdam wrote: strk wrote: On Wed, May 13, 2009 at 12:19:23AM -0400, Frank Warmerdam wrote: Only, it'll have to stay (I've read Mateusz suggesting using type-specific destroyers). I cannot imagine why a type specific destroyer would be needed for simple malloc()/free() buffer allocations. Frank, But GEOS user does not know what allocator is hidden behind GEOSGeomToWKT function and he does *not* need or want to know. GEOSFree exposes all the meat that is supposed be encapsulated and it makes GEOS C API not symmetric so confusing. Best regards, -- Mateusz Loskot, http://mateusz.loskot.net ___ geos-devel mailing list geos-devel@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/geos-devel
Re: [geos-devel] GEOS cross-heap problems
On May 13, 2009, at 7:42 AM, Mateusz Loskot wrote: Frank Warmerdam wrote: strk wrote: On Wed, May 13, 2009 at 12:19:23AM -0400, Frank Warmerdam wrote: Only, it'll have to stay (I've read Mateusz suggesting using type-specific destroyers). I cannot imagine why a type specific destroyer would be needed for simple malloc()/free() buffer allocations. Frank, But GEOS user does not know what allocator is hidden behind GEOSGeomToWKT function and he does *not* need or want to know. GEOSFree exposes all the meat that is supposed be encapsulated and it makes GEOS C API not symmetric so confusing. Aren't we supposed to be using the reader/writer API now? Add GEOSWK*Writer_Free functions (if we must) to those interfaces and GEOSFree to address the problem with deprecated GeomToWKT? Cheers, Sean ___ geos-devel mailing list geos-devel@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/geos-devel
Re: [geos-devel] GEOS cross-heap problems
Sean Gillies wrote: On May 13, 2009, at 7:42 AM, Mateusz Loskot wrote: Frank Warmerdam wrote: strk wrote: On Wed, May 13, 2009 at 12:19:23AM -0400, Frank Warmerdam wrote: Only, it'll have to stay (I've read Mateusz suggesting using type-specific destroyers). I cannot imagine why a type specific destroyer would be needed for simple malloc()/free() buffer allocations. Frank, But GEOS user does not know what allocator is hidden behind GEOSGeomToWKT function and he does *not* need or want to know. GEOSFree exposes all the meat that is supposed be encapsulated and it makes GEOS C API not symmetric so confusing. Aren't we supposed to be using the reader/writer API now? Add GEOSWK*Writer_Free functions (if we must) to those interfaces and GEOSFree to address the problem with deprecated GeomToWKT? Sean / Mateusz, It is my personal opinion that since these buffers are just allocated from the heap with malloc(), exposing a GEOS specific free - and making it clear when it is suitable to use is a more than adequate solution. If you guys feel strongly enough about type specific, or reader/writer interface specific deallocators for these buffers then I'd suggest you bring it to a motion to replace/override my implementation and I'll then adapt GDAL appropriately. I'm in a bad mood, and less receptive than normal to esoteric conceptual correctness wanking. Regards, -- ---+-- I set the clouds in motion - turn up | Frank Warmerdam, warmer...@pobox.com light and sound - activate the windows | http://pobox.com/~warmerdam and watch the world go round - Rush| Geospatial Programmer for Rent ___ geos-devel mailing list geos-devel@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/geos-devel
Re: [geos-devel] GEOS cross-heap problems
Frank Warmerdam wrote: Folks, I had a serious problem reported by a client of mine. The issue turned out to be that the OSGeo4W GEOS package was built with a different version of visual studio than the OSGeo4W GDAL package. The GDAL package calls: pabyBuf = GEOSGeomToWKB_buf( geosGeom, nSize ); ... free( pabyBuf ); The problem is that the buffer allocated by GEOS c-runtime is being freed with the free() of the GDAL c-runtime resulting in heap corruption. For things like geometry and coordinate sequences we have special deallocators available as part of the GEOS C API. However this is not the case (as far as I know) for plain malloc()/free() type stuff. So my questions are: 1) Am I missing an existing GEOSFree() function? Folks, Howard has pointed out that this is already a known issue, listed in: http://trac.osgeo.org/geos/ticket/249 I see Paul has accepted the ticket, but I don't see any sign that it has been incorporated. Also, what constraints do we have on backporting an API *addition* like this? Best regards, -- ---+-- I set the clouds in motion - turn up | Frank Warmerdam, warmer...@pobox.com light and sound - activate the windows | http://pobox.com/~warmerdam and watch the world go round - Rush| Geospatial Programmer for Rent ___ geos-devel mailing list geos-devel@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/geos-devel
Re: [geos-devel] GEOS cross-heap problems
Frank, Please feel free to add the patch into trunk. Regarding API additions, I am feeling a little conservative. At a minimum, once you start doing that, all the downstream packages have to start testing for revision in order to know if they can call the feature. I'd like to hear from others on this. On a related note, strk has completed a large run through upgrading GEOS to the latest JTS, which has added some extra robustness to a number of cases, and he's also tracked and killed some long-standing bugs. A GEOS 3.2 would not be a bad thing to have, rather than letting the functionality moulder in SVN. (This would also be a fine place for API changes) P. On Tue, May 12, 2009 at 1:47 PM, Frank Warmerdam warmer...@pobox.com wrote: Frank Warmerdam wrote: Folks, I had a serious problem reported by a client of mine. The issue turned out to be that the OSGeo4W GEOS package was built with a different version of visual studio than the OSGeo4W GDAL package. The GDAL package calls: pabyBuf = GEOSGeomToWKB_buf( geosGeom, nSize ); ... free( pabyBuf ); The problem is that the buffer allocated by GEOS c-runtime is being freed with the free() of the GDAL c-runtime resulting in heap corruption. For things like geometry and coordinate sequences we have special deallocators available as part of the GEOS C API. However this is not the case (as far as I know) for plain malloc()/free() type stuff. So my questions are: 1) Am I missing an existing GEOSFree() function? Folks, Howard has pointed out that this is already a known issue, listed in: http://trac.osgeo.org/geos/ticket/249 I see Paul has accepted the ticket, but I don't see any sign that it has been incorporated. Also, what constraints do we have on backporting an API *addition* like this? Best regards, -- ---+-- I set the clouds in motion - turn up | Frank Warmerdam, warmer...@pobox.com light and sound - activate the windows | http://pobox.com/~warmerdam and watch the world go round - Rush | Geospatial Programmer for Rent ___ geos-devel mailing list geos-devel@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/geos-devel ___ geos-devel mailing list geos-devel@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/geos-devel
Re: [geos-devel] GEOS cross-heap problems
Paul Ramsey wrote: Frank, Please feel free to add the patch into trunk. Paul, Done (r2506). Details at: http://trac.osgeo.org/geos/ticket/249 Regarding API additions, I am feeling a little conservative. At a minimum, once you start doing that, all the downstream packages have to start testing for revision in order to know if they can call the feature. I'd like to hear from others on this. OK - though I don't see the downside. Downstream packages only need to check for, and use GEOSFree() if they want. We are just adding a new option, not altering an existing one or removing anything. On a related note, strk has completed a large run through upgrading GEOS to the latest JTS, which has added some extra robustness to a number of cases, and he's also tracked and killed some long-standing bugs. A GEOS 3.2 would not be a bad thing to have, rather than letting the functionality moulder in SVN. (This would also be a fine place for API changes) I'd be pleased to see GEOS 3.2 though for my purposes (OSGeo4W) I would really like GEOSFree() in GEOS 3.0.4. Best regards, -- ---+-- I set the clouds in motion - turn up | Frank Warmerdam, warmer...@pobox.com light and sound - activate the windows | http://pobox.com/~warmerdam and watch the world go round - Rush| Geospatial Programmer for Rent ___ geos-devel mailing list geos-devel@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/geos-devel
Re: [geos-devel] GEOS cross-heap problems
Other comments? I have only vague discomfort, so in the absence of someone else having issues, I will subordinate my theoretical musing to Frank's practical need. P. On Tue, May 12, 2009 at 9:19 PM, Frank Warmerdam warmer...@pobox.com wrote: Paul Ramsey wrote: Frank, Please feel free to add the patch into trunk. Paul, Done (r2506). Details at: http://trac.osgeo.org/geos/ticket/249 Regarding API additions, I am feeling a little conservative. At a minimum, once you start doing that, all the downstream packages have to start testing for revision in order to know if they can call the feature. I'd like to hear from others on this. OK - though I don't see the downside. Downstream packages only need to check for, and use GEOSFree() if they want. We are just adding a new option, not altering an existing one or removing anything. On a related note, strk has completed a large run through upgrading GEOS to the latest JTS, which has added some extra robustness to a number of cases, and he's also tracked and killed some long-standing bugs. A GEOS 3.2 would not be a bad thing to have, rather than letting the functionality moulder in SVN. (This would also be a fine place for API changes) I'd be pleased to see GEOS 3.2 though for my purposes (OSGeo4W) I would really like GEOSFree() in GEOS 3.0.4. Best regards, -- ---+-- I set the clouds in motion - turn up | Frank Warmerdam, warmer...@pobox.com light and sound - activate the windows | http://pobox.com/~warmerdam and watch the world go round - Rush | Geospatial Programmer for Rent ___ geos-devel mailing list geos-devel@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/geos-devel ___ geos-devel mailing list geos-devel@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/geos-devel
