Fix Z3 refcount leak in simple backend _sym_push_path_constraint - #9
Open
Emad-Mahmodi wants to merge 1 commit into
Open
Fix Z3 refcount leak in simple backend _sym_push_path_constraint#9Emad-Mahmodi wants to merge 1 commit into
Emad-Mahmodi wants to merge 1 commit into
Conversation
newConstraint is inc_ref'd before being asserted but never dec_ref'd, so each call leaks one Z3_ast reference. Add the matching Z3_dec_ref; the solver keeps its own internal reference to the asserted constraint, so this is behavior-preserving (verified: identical diverging inputs before and after). Reported in eurecom-s3/symcc#180.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the reference-count bug reported in eurecom-s3/symcc#180.
In the simple backend's
_sym_push_path_constraint, the simplifiedconstraint(inc_ref'd) and its negationnot_constraint(inc_ref'd) are created, then one of them is selected asnewConstraintand inc_ref'd a second time before being asserted:The two
Z3_dec_refcalls balanceconstraintandnot_constraint, but the extrainc_refonnewConstrainthas no matchingdec_ref, so one AST reference leaks per call. Refcount trace (taken == true, sonewConstraint == constraint):The fix adds the matching
Z3_dec_ref(g_context, newConstraint);. The solver retains its own internal reference to the asserted constraint, so dropping the user reference to 0 keeps it asserted — the change is behavior-preserving. Verified: on a workload of 256 path constraints the set of diverging inputs found is byte-for-byte identical before and after the patch.Honest note on impact: in the simple backend
g_solveris never reset and the path constraint is asserted permanently, so the leaked reference is redundant with the solver's own retention and I did not observe a peak-RSS difference at this scale. This is therefore a correctness fix (the inc_ref/dec_ref discipline is simply wrong as written) rather than a measured performance fix; it matters in any context where the solver is reset/popped or where this pattern is reused.