Opened 4 years ago

Closed 2 years ago

Last modified 2 years ago

#3246 closed defect (fixed)

lwcollection_extract return object possibly referencing portions of argument

Reported by: strk Owned by: pramsey
Priority: blocker Milestone: PostGIS 2.4.0
Component: postgis Version: 2.1.x
Keywords: Cc:

Description

Like with #3245, lwcollection_extract is another function that might return objects that depend on memory owned by its arguments. It uses lwgeom_clone over the input components, rather than lwgeom_clone_deep.

Probably since 2.0.

Change History (12)

comment:1 Changed 4 years ago by pramsey

So, the question is kind of whether we feel OK having those references or not. It's certainly a lot lighter weight to do them, and I confirmed that, in ordinary usage, shallow cloning works OK:

static void test_clone(void)
{
	static char *wkt = "GEOMETRYCOLLECTION(MULTIPOLYGON(((0 0, 10 0, 10 10, 0 10, 0 0))),POINT(1 1),LINESTRING(2 3,4 5))";
	LWGEOM *geom1 = lwgeom_from_wkt(wkt, LW_PARSER_CHECK_ALL);
	LWGEOM *geom2;
	
	/* Free in "backwards" order */
	geom2 = lwgeom_clone(geom1);
	lwgeom_free(geom1);
	lwgeom_free(geom2);

	/* Free in "forewards" order */
	geom1 = lwgeom_from_wkt(wkt, LW_PARSER_CHECK_ALL);
	geom2 = lwgeom_clone(geom1);
	lwgeom_free(geom2);
	lwgeom_free(geom1);
}

Mind you, freeing the input and then *using* the output would fail, but in terms of freeing, the read-only guard works fine, there's no doublefree condition. Question is basically if we want to apply a "mutation implies copy" policy over the whole codebase or not, even when the mutation doesn't actually affect the coordinates (the biggest overhead of our memory use).

comment:2 Changed 4 years ago by robe

Milestone: PostGIS 2.1.9PostGIS 2.3.0

Definitely not something we want to see a change in 2.1.

FWIW: I'm all for keeping shallow cloning particularly where changing to deep could cause a performance degradation.

comment:3 Changed 3 years ago by robe

Milestone: PostGIS 2.3.0PostGIS 2.4.0

comment:4 Changed 2 years ago by dbaston

Priority: mediumblocker

Resolve this by adding a comment to the source explaining that return references portions of argument.

comment:5 Changed 2 years ago by robe

Milestone: PostGIS 2.4.0PostGIS 2.5.0

comment:6 Changed 2 years ago by pramsey

Resolution: fixed
Status: newclosed

I've noted in the code that the return value references portions of the argument. r15643

comment:7 Changed 2 years ago by robe

Milestone: PostGIS 2.5.0PostGIS 2.4.0

comment:8 Changed 2 years ago by robe

Resolution: fixed
Status: closedreopened

not to be picky here but it looks like you did a little more than just put a comment in here. You changed code. Not sure what to say in the NEWS about this but feels like something needs to be said.

comment:9 Changed 2 years ago by strk

Yup, sounds like a leak was added ?

comment:10 Changed 2 years ago by pramsey

Well yes, looking at the function, once you start having an eye for the kinds of leaks that hang around the whole lwgeom_release() pattern, you see some others, so I removed them since I was there. Like most of our leaks, they are leaking into the pgsql bathtub, which is drained at the end of the function call anyways :) It's not something users would ever perceive.

comment:11 Changed 2 years ago by pramsey

Resolution: fixed
Status: reopenedclosed

comment:12 Changed 2 years ago by strk

Chances are the memory will be released at the end of *statment*, not function call. May grow quick.

Note: See TracTickets for help on using tickets.