Yul optimizer: popScope omits variableChanged() on KnowledgeBase — sibling of PR #17116 (and 865543a6)

Open
  • Is something wrong?
#17,174 0 comments 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 1 day

Nobody has claimed this yet.

Assessment

Difficulty
2/5
Estimated time
1-3 hours
Newbie friendliness
55/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
cpp
Domain
compilers

Research direction

Start in libyul/optimiser/DataFlowAnalyzer.cpp, where popScope() erases m_state.value and m_state.sortedReferences without calling m_knowledgeBase.variableChanged(). Compare it with clearValues(), which already makes that call, and check libyul/optimiser/Suite.cpp to confirm the reachability claim. Done means the invalidation is consistent across the mutators and the existing Yul optimizer tests still pass.

Written by the indexing model from the issue text.

Description

Summary

PR #17116 (merged 2026-10-14) + its parent commit 865543a6 fixed YulOptimizerKnowledgeBaseMissesStaleBase by versioning KnowledgeBase's offset cache (keyed by (name, version) instead of just name) and adding m_knowledgeBase.variableChanged(name) calls to DataFlowAnalyzer::clearValues().

The sibling path DataFlowAnalyzer::popScope() (currently libyul/optimiser/DataFlowAnalyzer.cpp:290-299) erases m_state.value[name] but does not call variableChanged():

void DataFlowAnalyzer::popScope()
{
    for (auto const& name: m_variableScopes.back().variables)
    {
        m_state.value.erase(name);
        m_state.sortedReferences.erase(name);
        // ← missing: m_knowledgeBase.variableChanged(name);
    }
    m_variableScopes.pop_back();
}

grep -rn "m_state\.value\.erase\|m_state\.value\[" libyul/optimiser/DataFlowAnalyzer.cpp returns three erase/assign sites: clearValues() has the variableChanged call added by the fix; popScope() and assignValue() do not. assignValue() is called from handleAssignment() which runs clearValues(_variables) first on non-declarations, so it's covered via that path. popScope() is not covered.

Why it may matter (bounded)

If a later scope reuses the same YulName (which is possible pre-Disambiguator or in non-standard pipeline configurations), KnowledgeBase's m_currentVersions[name] stays at the old value, and explore(name) would return the stale cached offset computed in the earlier scope's lifetime. The yulAssert(cachedOffset->computedFrom == assignedValue) in explore() would catch pointer mismatch in debug; in release where assertions are stripped, same miscompile class as PR #17116.

Reachability caveat (honest)

libyul/optimiser/Suite.cpp:115 runs Disambiguator unconditionally before every user-controllable optimizer step. --yul-optimizations appends to the sequence AFTER Disambiguator and cannot schedule VarNameCleaner (not in stepNameToAbbreviationMap). So from the shipped solc binary, the sibling is not reachable: Disambiguator guarantees unique YulName across the function/block, defusing the trigger.

Reaching this would need a custom libyul consumer that bypasses Disambiguator — not a security scenario for shipped solc. Filing as code-review / defense-in-depth rather than bug.

Secondary observation

Commit 865543a6 is a second parent-fix in the same KnowledgeBase area that also missed popScope(). Two independent parent fixes in the same area closing adjacent but not this spot slightly raise the prior that it's an oversight rather than a design choice.

Suggested fix (one-line defense-in-depth)

void DataFlowAnalyzer::popScope()
{
    for (auto const& name: m_variableScopes.back().variables)
    {
        m_state.value.erase(name);
        m_state.sortedReferences.erase(name);
        m_knowledgeBase.variableChanged(name);  // match clearValues invariant
    }
    m_variableScopes.pop_back();
}

Tightens the invariant across all m_state.value mutators even though the shipped-binary flow is already safe via Disambiguator.

References

  • Fix PR: #17116 (merged 2026-10-14)
  • Parent fix commit: 865543a6 "Yul Optimizer: Knowledge uses versioned variable names"
  • Current main reference: libyul/optimiser/DataFlowAnalyzer.cpp:290-299
  • Reachability guard: libyul/optimiser/Suite.cpp:115 (Disambiguator always runs)

Reporter: symposia-gi (General Imperative, Symposia). Code-review observation only, defense-in-depth. Not a security report.

Dominant language
C++
Stars
25.8k
Forks
6.2k
Avg merge
1d 22h
Merged PRs (30d)
43

Getting set up

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from argotorg/solidity

All issues in argotorg/solidity

Similar issues

More C++ issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.