Opened 8 years ago
Closed 6 years ago
Last modified 6 years ago
#3246 closed defect (fixed)
lwcollection_extract return object possibly referencing portions of argument
|Reported by:||strk||Owned by:||pramsey|
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 by , 8 years ago
comment:2 by , 8 years ago
|Milestone:||PostGIS 2.1.9 → PostGIS 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 by , 7 years ago
|Milestone:||PostGIS 2.3.0 → PostGIS 2.4.0|
comment:4 by , 6 years ago
|Priority:||medium → blocker|
Resolve this by adding a comment to the source explaining that return references portions of argument.
comment:5 by , 6 years ago
|Milestone:||PostGIS 2.4.0 → PostGIS 2.5.0|
comment:6 by , 6 years ago
|Status:||new → closed|
I've noted in the code that the return value references portions of the argument. r15643
comment:7 by , 6 years ago
|Milestone:||PostGIS 2.5.0 → PostGIS 2.4.0|
comment:8 by , 6 years ago
|Status:||closed → reopened|
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 by , 6 years ago
Yup, sounds like a leak was added ?
comment:10 by , 6 years ago
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 by , 6 years ago
|Status:||reopened → closed|
comment:12 by , 6 years ago
Chances are the memory will be released at the end of *statment*, not function call. May grow quick.
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:
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).