Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix leak on projected heap-allocated graph entities #996

Merged
merged 3 commits into from
Mar 3, 2020
Merged

Fix leak on projected heap-allocated graph entities #996

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a392204
Fix leak on projected heap-allocated graph entities
jeffreylovitz Feb 24, 2020
34bda88
Merge branch 'master' into free-heap-graph-entities
swilly22 Mar 2, 2020
cdc98d0
Merge branch 'master' into free-heap-graph-entities
swilly22 Mar 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/execution_plan/ops/op_project.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ static Record ProjectConsume(OpBase *opBase) {
* TODO This is a rare case; the logic of when to persist can be improved. */
if(!(v.type & SI_GRAPHENTITY)) SIValue_Persist(&v);
Record_Add(op->projection, rec_idx, v);
/* If the value was a graph entity with its own allocation, as with a query like:
* MATCH p = (src) RETURN nodes(p)[0]
* Ensure that the allocation is freed here. */
if((v.type & SI_GRAPHENTITY)) SIValue_Free(v);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please elaborate on how v is constructed in this scenario

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can, but the arithmetic expression tree that returns v in my example is pretty complicated:
subscript(nodes(topath(POINTER,src)),0)

Here's a more readable case with the same behavior:
MATCH (p) WITH COLLECT(p) AS list RETURN list[0]
Our leak is on list[0], which is the simpler expression subscript(list,0).

AR_SUBSCRIPT returns a clone of the SIValue within list, giving us a heap-allocated node in SIValue v.

Since adding a graph entity to a record doesn't rely on the heap (direct assignment to the internal Entry), v is a dangling pointer within the projection loop.

I tried a few methods to remove the clone from the subscript function, but there seem to be cases where it is necessary for non-entity values. The differences between our ownership mechanisms for Record graph entities vs. SIValues, at present, require some slightly wonky logic like this.

}

OpBase_DeleteRecord(op->r);
Expand Down