Opened 6 years ago

Closed 6 years ago

#3569 closed defect (fixed)

ST_CollectionHomogenize sometimes returns reference to argument

Reported by: dbaston Owned by: pramsey
Priority: blocker Milestone: PostGIS 2.2.3
Component: postgis Version: 2.2.x
Keywords: Cc:

Description

I noticed this situation when digging into a question posted on Stack Exchange: http://gis.stackexchange.com/questions/195915/weird-behavior-with-st-clusterwithin

That question itself isn't relevant here, but the some of the test data can be used to make a reproducible (for me) test case of the problem:

CREATE TABLE cluster_inputs (
    name text,
    geom geometry
);

CREATE TABLE cluster (
    geom geometry
);

INSERT INTO cluster (geom) VALUES ('010700002031BF0D000100000001030000000100000005000000713D0AD76D4D11C1A4703D9AE5955941713D0AD76D4D11C1A4703D9A94965941713D0AD77D4211C1A4703D9A94965941713D0AD77D4211C1A4703D9AE5955941713D0AD76D4D11C1A4703D9AE5955941');
INSERT INTO cluster (geom) VALUES ('010700002031BF0D00010000000103000000010000000D0000009A9999998B3211C114AE47F13D965941295C8FC2863211C1A4703D5A569659418FC2F528D33011C1EC51B84E5A9659417B14AE47B62F11C1B81E851B5A965941295C8FC2BC2F11C18FC2F5F85D96594114AE47E10D2C11C1295C8FE26F965941EC51B81EF32B11C1666666D6699659417B14AE47D12C11C1F6285CFF139659418FC2F528BA2F11C1F6285C7F14965941B81E85EBBE2F11C11F85EB01279659415C8FC2F5DB2F11C1D7A3702D2C965941F6285C8F2F3011C15C8FC235309659419A9999998B3211C114AE47F13D965941');


INSERT INTO cluster_inputs (name, geom) VALUES ('a', '010300002031BF0D000100000005000000713D0AD76D4D11C1A4703D9AE5955941713D0AD76D4D11C1A4703D9A94965941713D0AD77D4211C1A4703D9A94965941713D0AD77D4211C1A4703D9AE5955941713D0AD76D4D11C1A4703D9AE5955941');
INSERT INTO cluster_inputs (name, geom) VALUES ('b', '010300002031BF0D00010000000D0000009A9999998B3211C114AE47F13D965941295C8FC2863211C1A4703D5A569659418FC2F528D33011C1EC51B84E5A9659417B14AE47B62F11C1B81E851B5A965941295C8FC2BC2F11C18FC2F5F85D96594114AE47E10D2C11C1295C8FE26F965941EC51B81EF32B11C1666666D6699659417B14AE47D12C11C1F6285CFF139659418FC2F528BA2F11C1F6285C7F14965941B81E85EBBE2F11C11F85EB01279659415C8FC2F5DB2F11C1D7A3702D2C965941F6285C8F2F3011C15C8FC235309659419A9999998B3211C114AE47F13D965941');

For me, running the following query then produces an error:

SELECT
  cluster_inputs.name, 
  ST_Intersects(ST_CollectionHomogenize(cluster.geom), cluster_inputs.geom)
FROM cluster_inputs, cluster
ORDER BY name;

ERROR:  First argument geometry could not be converted to GEOS: IllegalArgumentException: Points of LinearRing do not form a closed linestring

which suggests a memory issue. The problem can be resolved by wrapping the return of lwgeom_homogenize with an lwgeom_clone_deep, which seems to confirm this. Since the return of lwgeom_homogenize references portions of its argument, and ST_CollectionHomogenize releases the serialized input before serializing the output, we seem to have a problem.

Attachments (1)

3569.patch (1.5 KB ) - added by dbaston 6 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 by Mike Taves, 6 years ago

Here's a simplified version of the issue:

WITH cluster AS (
  SELECT ST_Polygonize(ST_MakeEnvelope(1, 2, 3, 4)) AS geom
)
SELECT ST_GeometryType(geom),
  ST_CollectionHomogenize(geom)::box2d,
  ST_CollectionHomogenize(geom::text)::box2d
FROM cluster;

-[ RECORD 1 ]-----------+----------------------
st_geometrytype         | ST_GeometryCollection
st_collectionhomogenize | BOX(0 0,3 4)
st_collectionhomogenize | BOX(1 2,3 4)

The error is shown from the second field, where the X and Y coords are 0. A workaround is shown in the last example, where the geometry is recreated.

comment:2 by robe, 6 years ago

Priority: highblocker

Okay this is a problem in both 2.2 and 2.3.

Does anyone have issue with applying dbaston's suggestion. If not I'm going to go ahead and do that.

dbaston,

I'd feel better if you did it now that you have commit rights :).

Last edited 6 years ago by robe (previous) (diff)

comment:3 by pramsey, 6 years ago

Fixing homogenize to return a full copy seems preferable to deep cloning the return from homogenize, ISTM.

in reply to:  3 comment:4 by dbaston, 6 years ago

Replying to pramsey:

Fixing homogenize to return a full copy seems preferable to deep cloning the return from homogenize, ISTM.

Agreed. I don't think we actually need a clone though, I think it's sufficient to just avoid freeing our input GSERIALIZED before we've serialized our output. If there are no concerns with the attached patch that does this, I'll commit it.

by dbaston, 6 years ago

Attachment: 3569.patch added

comment:5 by pramsey, 6 years ago

Sure. Maybe add a TODO comment in the code indicating we're guarding against this problem and should maybe change the liblwgeom contract.

comment:6 by dbaston, 6 years ago

Fixed in trunk at r15105 and 2.2 at r15107.

comment:7 by dbaston, 6 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.