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.

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

Fix a double free of the implicit variable #50

Open
wants to merge 1 commit into
base: future
Choose a base branch
from

Conversation

leyarotheconquerer
Copy link
Contributor

@leyarotheconquerer leyarotheconquerer commented Jul 4, 2018

Attempts to fix #47

A raw pointer to the implicit variable is returned from interpretExprNode.
Most expressions free ValueObjects returned from interpretExprNode after
using the value. This resulted in the implicit variable being deleted after
its first use.

When the scope object is cleaned up, implicit variable is deleted again,
resulting in a double free. There are two simple solutions to this:

  1. Set the implicit variable pointer in the scope to NULL after it's used in the
    expression.
  2. Use a copy of the implicit variable in the expression.

I chose the latter because it seemed simpler and didn't destroy any data.

A test has also been added to catch regressions, though it only tests the
addition operator.

A raw pointer to the implicit variable is returned from `interpretExprNode`.
Most expressions free ValueObjects returned from `interpretExprNode` after
using the value. This resulted in the implicit variable being deleted after
its first use.

When the scope object is cleaned up, implicit variable is deleted again,
resulting in a double free. There are two simple solutions to this:

1. Set the implicit variable pointer in the scope to NULL after it's used in the
expression.
2. Use a copy of the implicit variable in the expression.

I chose the latter because it seemed simpler and didn't destroy any data.
Hopefully it doesn't lead to memory leaks. The one interpret statement that
doesn't delete expression values after use is `interpretSystemCommandExprNode`.
Aside from that function, this should be a safe change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant