Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#2293 closed defect (fixed)

crash on ST_DumpPoints with curved geoms

Reported by: robe Owned by: nw
Priority: blocker Milestone: PostGIS 2.1.0
Component: postgis Version: master
Keywords: Cc: nw@…

Description

I started running thru my garden tests. I'll double check my old test. But I started testing using r10363 on 9.2 edb win 32bit and that went thru without crashing.

Then I upgraded to latest:

POSTGIS="2.1.0SVN r11330" GEOS="3.4.0dev-CAPI-1.8.0 r0" PROJ="Rel. 4.8.0, 6 March 2012" GDAL="GDAL 1.10.0, released 2013/04/24 GDAL_DATA not found" LIBXML="2.7.8" LIBJSON="UNKNOWN" RASTER PostgreSQL 9.2.0, compiled by Visual C++ build 1600, 32-bit

Crashola on this query:

SELECT ST_DumpPoints(foo1.the_geom) As result
	FROM ((SELECT ST_GeomFromEWKT('SRID=4326;CURVEPOLYGON(CIRCULARSTRING(-71.0821 42.3036, -71.4821 42.3036, -71.7821 42.7036, -71.0821 42.7036, -71.0821 42.3036),(-71.1821 42.4036, -71.3821 42.6036, -71.3821 42.4036, -71.1821 42.4036) ) ') As the_geom)) As foo1 LIMIT 3;

I suspect this may be caused by recent improvements on curved support.

Attachments (1)

dumppoints.patch (2.1 KB ) - added by nw 11 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 by robe, 11 years ago

actually might be caused by the making of ST_DumpPoints a native C function. I realized the pre-version I was testing with r10363 (that did not crash) was about a month or so before ST_DumpPoints was flipped to a C function.

comment:2 by strk, 11 years ago

Confirmed crash with POSTGIS="2.1.0SVN r11321" on PostgreSQL 9.1.9 on x86_64-unknown-linux-gnu, compiled by gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3, 64-bit

comment:3 by robe, 11 years ago

hmm — you suppose this TODO error: is a sign from somebody?

http://postgis.net/docs/doxygen/2.1/dc/d01/lwgeom__dumppoints_8c_source.html#00201

case CURVEPOLYTYPE:
                                default:
                                        /* TODO error? */
                                        if (--state->stacklen == 0) SRF_RETURN_DONE(funcctx);
                                        state->pathlen--;
                                        continue;

comment:5 by strk, 11 years ago

it sounds like there should be a break in there, substituting the three lines under the TODO. Not that the break should affect the crash (culprit doesn't seem to be there). Valgrind would help here.

comment:6 by robe, 11 years ago

Cc: nw@… added

Nathan,

Can you look at this one and provide a patch if you can. we'd like to have this resolved before we call our first 2.1 beta in prep for 9.3 beta1.

comment:7 by nw, 11 years ago

Owner: changed from pramsey to nw
Status: newassigned

Will do. IIRC, the original function essentially ignored curved types. What exactly *should* be the behavior here? Dump what I assume are the control points? Return NULL? Return an empty array? Throw an exception? Interpolate points along the curve?

comment:8 by strk, 11 years ago

I think you should dump the points (which I believe are 3 points-on-curve per segment). But the crash suggests there's a memory error somewhere, as it should still be possible to ignore unsupported geometries without crashing. So I think you should first make it stable w/out supporting curves and only later, if you want, add support for the curve.

in reply to:  3 comment:9 by nw, 11 years ago

Replying to robe:

hmm — you suppose this TODO error: is a sign from somebody?

Actually, that was me wondering if st_dumppoints() should throw an error rather than just ignoring geometries it didn't understand.

comment:10 by robe, 11 years ago

it should throw an error

by nw, 11 years ago

Attachment: dumppoints.patch added

comment:11 by nw, 11 years ago

I have attached a patch made with 'git diff' that adds two tests on curved geometry to the dumppoints regression test, and changes the behavior to throw a data exception in case of unknown or unhandled geometries. It passes a make check on my system, which is postgresql 9.2.4, on a 64 bit AMD linux system. I don't think there's anything about the code that would cause it to behave differently in a different environment.

I would be happy to have it dump out the control points for curved geoms, but (per suggestion) let's open a different ticket for that.

Given that I tried to duplicate exactly the behavior of the plpgsql dumppoints function, I think that throwing an error here may be a change in behavior. In any case, the behavior should be documented. I can submit a doc patch if needed and desired, though I will need to install some more software to build the docs.

comment:12 by robe, 11 years ago

That's fine (I can add that to disclaimer of docs). I thought the old ST_DumpPoints did dump curved geoms so that sounds like a breaking change. I could be wrong though. I'll test on my old 2.0 and if that is the case, we'd probably want to get it in 2.1 (since it doesn't change the API and is a breaking change from 2.0), it would be marked as a bug fix rather than an enhancement so doesn't have to be done before our 2.1 beta release.

comment:13 by robe, 11 years ago

Resolution: fixed
Status: assignedclosed

committed at r11346. I'll update the docs next and create a separate ticket to support curved geoms.

comment:14 by robe, 11 years ago

Okay I was mistaken — i guess 2.0 did not support this either though throws this error:

ERROR:  Unexpected error while dumping geometry CIRCULARSTRING(-71.0821 42.3036,-71.4821 42.3036,-71.7821 42.7036,-71.0821 42.7036,-71.0821 42.3036)
CONTEXT:  PL/pgSQL function _st_dumppoints(geometry,integer[]) line 22 at FOR over SELECT rows
SQL function "st_dumppoints" statement 1

I'll add curved support as a separate ticket.

Note: See TracTickets for help on using tickets.