crash on ST_DumpPoints with curved geoms

[robe]

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.

[robe]

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.

[strk]

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

[robe]

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;

[robe]

oops screwed up the link ​http://postgis.net/docs/doxygen/2.1/dc/d01/lwgeom__dumppoints_8c_source.html#l00201

[strk]

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.

[robe]

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.

[nw]

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?

[strk]

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.

[nw]

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.

[robe]

it should throw an error

[nw]

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.

[robe]

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.

[robe]

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

[robe]

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.