From 435af6afe6a4ce1f0775818ce3273e0b005e404b Mon Sep 17 00:00:00 2001 From: em2machine <92717390+em2machine@users.noreply.github.com> Date: Sat, 26 Sep 2026 00:54:53 +0200 Subject: [PATCH] Internals: Coverage cleanups before UAF fix (#8494) --- src/V3LinkDot.cpp | 24 ++--- src/V3LinkDotIfaceCapture.cpp | 116 +++++------------------- src/V3LinkDotIfaceCapture.h | 38 ++------ src/V3Param.cpp | 54 +---------- test_regress/t/t_iface_capture_debug.py | 21 +++++ 5 files changed, 60 insertions(+), 193 deletions(-) create mode 100755 test_regress/t/t_iface_capture_debug.py diff --git a/src/V3LinkDot.cpp b/src/V3LinkDot.cpp index 906565144..881efee0d 100644 --- a/src/V3LinkDot.cpp +++ b/src/V3LinkDot.cpp @@ -3358,8 +3358,7 @@ class LinkDotResolveVisitor final : public VNVisitor { // Capture a ParamTypeDType reference for interface typedef retargeting. // Called when a RefDType resolves to a ParamTypeDType owned by an interface. - void captureIfaceParamType(AstRefDType* nodep, AstParamTypeDType* defp, - const V3LinkDotIfaceCapture::CapturedEntry* capEntryp) { + void captureIfaceParamType(AstRefDType* nodep, AstParamTypeDType* defp) { if (!V3LinkDotIfaceCapture::enabled() || !m_statep->forPrimary()) return; AstNodeModule* const defOwnerModp = V3LinkDotIfaceCapture::findOwnerModule(defp); if (!defOwnerModp || !VN_IS(defOwnerModp, Iface)) return; @@ -3369,8 +3368,7 @@ class LinkDotResolveVisitor final : public VNVisitor { UINFO(9, indent() << "iface capture add paramtype " << nodep << " iface=" << defOwnerModp->prettyNameQ()); V3LinkDotIfaceCapture::addParamType(nodep, cellForCapture->name(), m_modp, defp, - defOwnerModp->name(), - capEntryp ? capEntryp->ifacePortVarp : nullptr); + defOwnerModp->name()); } AstNodeStmt* addImplicitSuperNewCall(AstFunc* const nodep, @@ -4780,9 +4778,8 @@ class LinkDotResolveVisitor final : public VNVisitor { refp->typedefp(defp); V3LinkDotIfaceCapture::captureTypedefContext( - refp, "typedef", static_cast(m_ds.m_dotPos), - m_ds.m_dotPos == DP_FINAL, m_ds.m_dotText, m_ds.m_dotSymp, m_curSymp, - m_modp, nodep, [this]() { return indent(); }); + refp, "typedef", static_cast(m_ds.m_dotPos), m_ds.m_dotText, + m_ds.m_dotSymp, m_modp, [this]() { return indent(); }); if (VN_IS(nodep->backp(), SelExtract)) { m_packedArrayDtp = refp; @@ -4799,9 +4796,8 @@ class LinkDotResolveVisitor final : public VNVisitor { refp->refDTypep(defp); V3LinkDotIfaceCapture::captureTypedefContext( - refp, "paramtype", static_cast(m_ds.m_dotPos), - m_ds.m_dotPos == DP_FINAL, m_ds.m_dotText, m_ds.m_dotSymp, m_curSymp, - m_modp, nodep, [this]() { return indent(); }); + refp, "paramtype", static_cast(m_ds.m_dotPos), m_ds.m_dotText, + m_ds.m_dotSymp, m_modp, [this]() { return indent(); }); if (VN_IS(nodep->backp(), SelExtract)) { m_packedArrayDtp = refp; @@ -6102,10 +6098,6 @@ class LinkDotResolveVisitor final : public VNVisitor { } // Resolve its reference - if (V3LinkDotIfaceCapture::find(nodep)) { - UINFO(9, indent() << "iface capture visit captured typedef ptr=" << nodep - << " user2=" << nodep->user2p()); - } if (m_statep->forParamed() && nodep->user3()) { if (V3LinkDotIfaceCapture::enabled() && nodep->user2p()) { UINFO(9, indent() << "iface capture clear user3 for captured typedef name=" @@ -6189,8 +6181,6 @@ class LinkDotResolveVisitor final : public VNVisitor { VL_DO_DANGLING(pushDeletep(cpackagep->unlinkFrBack()), cpackagep); } - const V3LinkDotIfaceCapture::CapturedEntry* capEntryp = V3LinkDotIfaceCapture::find(nodep); - if (m_ds.m_dotp && (m_ds.m_dotPos == DP_PACKAGE || m_ds.m_dotPos == DP_SCOPE)) { UASSERT_OBJ(VN_IS(m_ds.m_dotp->lhsp(), ClassOrPackageRef), m_ds.m_dotp->lhsp(), "Bad package link"); @@ -6271,7 +6261,7 @@ class LinkDotResolveVisitor final : public VNVisitor { } else { nodep->refDTypep(defp); nodep->classOrPackagep(foundp->classOrPackagep()); - captureIfaceParamType(nodep, defp, capEntryp); + captureIfaceParamType(nodep, defp); } } else if (AstClass* const defp = foundp ? VN_CAST(foundp->nodep(), Class) : nullptr) { diff --git a/src/V3LinkDotIfaceCapture.cpp b/src/V3LinkDotIfaceCapture.cpp index 79a859edc..14cefb5a1 100644 --- a/src/V3LinkDotIfaceCapture.cpp +++ b/src/V3LinkDotIfaceCapture.cpp @@ -19,9 +19,8 @@ // The IfaceCapture system has three phases with strict responsibilities: // // 1. CAPTURE (V3LinkDot, primary pass): -// add() / addParamType() / addTypedef() record template entries. -// Template entries store the REFDTYPE, its cellPath, and the -// original paramTypep / typedefp from the template module. +// add() / addParamType() / addClass() record template entries. +// Template entries store the REFDTYPE and its cellPath. // Template entries have cloneCellPath = "". // // 2. CLONE REGISTRATION (V3Param, deepCloneModule): @@ -30,8 +29,8 @@ // At this point the cloned module's cells still reference template // interface modules (cell->modp() is stale). Any attempt to walk // cellPath here finds the wrong module. Clone entries store the -// cloned REFDTYPE and cloneCellPath but clear paramTypep/typedefp -// so that stale template pointers are never carried forward. +// cloned REFDTYPE and cloneCellPath, and drop the template's extra +// REFDTYPEs so that stale template pointers are never carried forward. // // 3. TARGET RESOLUTION (finalizeIfaceCapture, after V3Param): // Runs after all cloning is complete and cell pointers are wired @@ -371,7 +370,7 @@ void V3LinkDotIfaceCapture::nullStaleLedgerRefs(const std::unordered_set") - << " refp=" << cvtToHex(entry.refp) << " cellPath='" << entry.cellPath << "'" - << " ownerMod=" << (entry.ownerModp ? entry.ownerModp->name() : "") - << " typedefp=" << (entry.typedefp ? entry.typedefp->name() : "") - << " typedefOwnerModName='" << entry.typedefOwnerModName << "'" - << " paramTypep=" << (entry.paramTypep ? entry.paramTypep->name() : "") - << " ifacePortVarp=" - << (entry.ifacePortVarp ? entry.ifacePortVarp->name() : "")); + UINFO(9, " [" << idx << "] " << captType << " key={" << key.ownerModName << "," + << key.refName << "," << key.cellPath << "," << key.cloneCellPath << "}" + << " ref=" << (entry.refp ? entry.refp->name() : "") << " refp=" + << cvtToHex(entry.refp) << " cellPath='" << entry.cellPath << "'" + << " ownerMod=" << (entry.ownerModp ? entry.ownerModp->name() : "") + << " typedefOwnerModName='" << entry.typedefOwnerModName << "'"); ++idx; } UINFO(9, "========== end iface capture dumpEntries =========="); } -string V3LinkDotIfaceCapture::extractIfacePortName(const string& dotText) { - string name = dotText; - const size_t dotPos = name.find('.'); - if (dotPos != string::npos) name = name.substr(0, dotPos); - const size_t braPos = name.find("__BRA__"); - if (braPos != string::npos) name = name.substr(0, braPos); - return name; -} - void V3LinkDotIfaceCapture::add(AstRefDType* refp, const string& cellPath, AstNodeModule* ownerModp, AstTypedef* typedefp, - const string& typedefOwnerModName, AstVar* ifacePortVarp) { + const string& typedefOwnerModName) { UASSERT(refp, "add() called with null refp"); UASSERT(ownerModp, "add() called with null ownerModp for refp=" << refp->prettyNameQ()); if (!typedefp) typedefp = refp->typedefp(); @@ -454,18 +439,9 @@ void V3LinkDotIfaceCapture::add(AstRefDType* refp, const string& cellPath, << refp->name() << " cellPath='" << cellPath << "'" << " ownerMod=" << ownerModName << " extraRefps.size=" << it->second.extraRefps.size()); } else { - s_map[key] = CapturedEntry{CaptureType::IFACE, - TargetKind::TYPEDEF, - refp, - cellPath, - /*cloneCellPath=*/"", - /*origClassp=*/nullptr, - ownerModp, - typedefp, - nullptr, - tdOwnerName, - ifacePortVarp, - {}}; + s_map[key] + = CapturedEntry{CaptureType::IFACE, TargetKind::TYPEDEF, refp, cellPath, + /*cloneCellPath=*/"", ownerModp, tdOwnerName, {}}; UINFO(9, "iface capture add: refp=" << refp->name() << " cellPath='" << cellPath << "'" << " ownerMod=" << ownerModName << " typedefp=" << (typedefp ? typedefp->name() : "") @@ -486,40 +462,13 @@ void V3LinkDotIfaceCapture::addClass(AstRefDType* refp, AstClass* origClassp, UASSERT_OBJ(!cellPath.empty(), origClassp, "addClass() produced empty cellPath"); const string ownerModName = ownerModp->name(); const CaptureKey key{ownerModName, refp->name(), cellPath, ""}; - s_map[key] = CapturedEntry{CaptureType::CLASS, - TargetKind::TYPEDEF, - refp, - cellPath, - /*cloneCellPath=*/"", - origClassp, - ownerModp, - typedefp, - nullptr, - tdOwnerName, - nullptr, - {}}; + s_map[key] = CapturedEntry{CaptureType::CLASS, TargetKind::TYPEDEF, refp, cellPath, + /*cloneCellPath=*/"", ownerModp, tdOwnerName, {}}; UINFO(9, "iface capture addClass: refp=" << refp->name() << " cellPath='" << cellPath << "'" << " ownerMod=" << (ownerModp ? ownerModp->name() : "")); } -// Not called in production - retained as a diagnostic/debug entry point -// for inspecting the capture ledger by key (e.g. from GDB or future code). -const V3LinkDotIfaceCapture::CapturedEntry* // LCOV_EXCL_START -V3LinkDotIfaceCapture::find(const CaptureKey& key) { - const auto it = s_map.find(key); - if (VL_UNLIKELY(it == s_map.end())) return nullptr; - return &it->second; -} // LCOV_EXCL_STOP - -const V3LinkDotIfaceCapture::CapturedEntry* V3LinkDotIfaceCapture::find(const AstRefDType* refp) { - if (!refp) return nullptr; - for (const auto& kv : s_map) { - if (kv.second.refp == refp) return &kv.second; - } - return nullptr; -} - // Walk a dot-separated cell path through the cell / IFACEREFDTYPE hierarchy // starting from startModp. Returns the module at the end of the path, or // nullptr if any component cannot be resolved. @@ -666,10 +615,8 @@ void V3LinkDotIfaceCapture::forEachOwned(const AstNodeModule* ownerModp, // replaces the lambda used in V3LinkDot.cpp for iface capture void V3LinkDotIfaceCapture::captureTypedefContext(AstRefDType* refp, const char* stageLabel, - int dotPos, bool /*dotIsFinal*/, - const std::string& dotText, VSymEnt* dotSymp, - VSymEnt* curSymp, AstNodeModule* modp, - AstNode* /*nodep*/, + int dotPos, const std::string& dotText, + VSymEnt* dotSymp, AstNodeModule* modp, const std::function& indentFn) { if (!enabled() || !refp) return; @@ -700,21 +647,11 @@ void V3LinkDotIfaceCapture::captureTypedefContext(AstRefDType* refp, const char* UASSERT_OBJ(!dotText.empty(), refp, "captureTypedefContext: dotText empty"); const string cellPath = dotText; - AstVar* ifacePortVarp = nullptr; - if (curSymp) { - const std::string portName = extractIfacePortName(dotText); - if (VSymEnt* const portSymp = curSymp->findIdFallback(portName)) { - ifacePortVarp = VN_CAST(portSymp->nodep(), Var); - UINFO(9, indentFn() << "iface capture found port var '" << portName << "' -> " - << ifacePortVarp); - } - } - // Check if refDTypep is a ParamTypeDType - if so, use addParamType instead of add if (AstParamTypeDType* const paramTypep = VN_CAST(refp->refDTypep(), ParamTypeDType)) { - V3LinkDotIfaceCapture::addParamType(refp, cellPath, modp, paramTypep, "", ifacePortVarp); + V3LinkDotIfaceCapture::addParamType(refp, cellPath, modp, paramTypep, ""); } else { - V3LinkDotIfaceCapture::add(refp, cellPath, modp, refp->typedefp(), "", ifacePortVarp); + V3LinkDotIfaceCapture::add(refp, cellPath, modp, refp->typedefp()); } UINFO(9, indentFn() << "iface capture capture success typedef=" << refp @@ -772,12 +709,8 @@ void V3LinkDotIfaceCapture::captureInnerParamTypeRefs(AstParamTypeDType* paramTy innerRefp, nestedCellName.empty() ? cellPath : nestedCellName, /*cloneCellPath=*/"", - /*origClassp=*/nullptr, ptOwnerModp, - innerRefp->typedefp(), - nullptr, refOwnerModp->name(), - nullptr, {}}; } } @@ -786,8 +719,7 @@ void V3LinkDotIfaceCapture::captureInnerParamTypeRefs(AstParamTypeDType* paramTy void V3LinkDotIfaceCapture::addParamType(AstRefDType* refp, const string& cellPath, AstNodeModule* ownerModp, AstParamTypeDType* paramTypep, - const string& paramTypeOwnerModName, - AstVar* ifacePortVarp) { + const string& paramTypeOwnerModName) { UASSERT(refp, "addParamType() called with null refp"); UASSERT(ownerModp, "addParamType() called with null ownerModp for refp='" << refp->prettyNameQ() << "'"); @@ -824,12 +756,8 @@ void V3LinkDotIfaceCapture::addParamType(AstRefDType* refp, const string& cellPa refp, cellPath, /*cloneCellPath=*/"", - /*origClassp=*/nullptr, ownerModp, - nullptr, - paramTypep, ptOwnerName, - ifacePortVarp, {}}; } diff --git a/src/V3LinkDotIfaceCapture.h b/src/V3LinkDotIfaceCapture.h index ea10099aa..7d561e921 100644 --- a/src/V3LinkDotIfaceCapture.h +++ b/src/V3LinkDotIfaceCapture.h @@ -1,9 +1,8 @@ // -*- mode: C++; c-file-style: "cc-mode" -*- //************************************************************************* // DESCRIPTION: Interface typedef capture helper. -// Stores (refp, typedefp, cellp, owners, pendingClone) so LinkDot can -// rebind refs when symbol lookup fails, and V3Param clones can retarget -// typedefs without legacy paths. +// Records RefDTypes that reach an interface typedef through a cell path, so +// V3Param clones can retarget them to the correct interface specialization. // // Code available from: https://verilator.org // @@ -78,17 +77,10 @@ public: string cellPath; // Template path (e.g. "cca_io.tlb_io") - immutable key component string cloneCellPath; // Instance-specific path (e.g. "cca_io1.tlb_io") - set by // propagateClone when V3Param clones; empty for original entries - AstClass* origClassp = nullptr; // For CLASS captures // Module where the RefDType lives AstNodeModule* ownerModp = nullptr; - // Typedef definition being referenced - AstTypedef* typedefp = nullptr; - // For PARAMTYPEDTYPE - AstParamTypeDType* paramTypep = nullptr; // Name of the module/interface that owns the typedef (stable string) string typedefOwnerModName; - // Interface port variable for matching during cloning - AstVar* ifacePortVarp = nullptr; // Additional REFDTYPEs sharing the same key (e.g. from macro expansions // that produce multiple $bits() references to the same interface typedef). // The primary refp is stored above; extras are appended here so that @@ -96,11 +88,7 @@ public: std::vector extraRefps; // Clear template-specific targets that are stale in a clone context. // Called by propagateClone before inserting a clone entry. - void clearStaleRefs() { - paramTypep = nullptr; - typedefp = nullptr; - extraRefps.clear(); - } + void clearStaleRefs() { extraRefps.clear(); } // Visit every AstNode* pointer field (analogous to AstNode::foreachLink). // The callback receives an AstNode* by reference; if it nulls the // pointer the typed member is nulled accordingly. @@ -113,10 +101,6 @@ public: }; callOnNode(refp); callOnNode(ownerModp); - callOnNode(typedefp); - callOnNode(paramTypep); - callOnNode(ifacePortVarp); - callOnNode(origClassp); for (auto& xrefp : extraRefps) callOnNode(xrefp); } }; @@ -134,7 +118,6 @@ private: static void reset(); static void clearModuleCache(); static AstIfaceRefDType* ifaceRefFromVarDType(AstNodeDType* dtypep); - static string extractIfacePortName(const string& dotText); // True if this module is a copy of that one. static bool isCloneOfModule(const AstNodeModule* modp, const AstNodeModule* templateModp); // Point a reference at a typedef and fix its other links. @@ -170,17 +153,11 @@ public: // Retarget every live RefDType in an entry using only stable capture metadata. static bool retargetRefToModule(const CapturedEntry& entry, AstNodeModule* targetModp); static void add(AstRefDType* refp, const string& cellPath, AstNodeModule* ownerModp, - AstTypedef* typedefp = nullptr, const string& typedefOwnerModName = "", - AstVar* ifacePortVarp = nullptr); + AstTypedef* typedefp = nullptr, const string& typedefOwnerModName = ""); static void addClass(AstRefDType* refp, AstClass* origClassp, AstNodeModule* ownerModp, AstTypedef* typedefp = nullptr, const string& typedefOwnerModName = ""); static void addParamType(AstRefDType* refp, const string& cellPath, AstNodeModule* ownerModp, - AstParamTypeDType* paramTypep, const string& paramTypeOwnerModName, - AstVar* ifacePortVarp); - // Exact lookup by full key - static const CapturedEntry* find(const CaptureKey& key); - // Pointer-based lookup: linear scan with early exit (no std::function overhead) - static const CapturedEntry* find(const AstRefDType* refp); + AstParamTypeDType* paramTypep, const string& paramTypeOwnerModName); static void forEach(const std::function& fn); static void forEachOwned(const AstNodeModule* ownerModp, const std::function& fn); @@ -198,9 +175,8 @@ public: AstNodeModule* newOwnerModp, const string& cloneCellPath); static void captureTypedefContext(AstRefDType* refp, const char* stageLabel, int dotPos, - bool dotIsFinal, const std::string& dotText, - VSymEnt* dotSymp, VSymEnt* curSymp, AstNodeModule* modp, - AstNode* nodep, + const std::string& dotText, VSymEnt* dotSymp, + AstNodeModule* modp, const std::function& indentFn); // Null out ledger entries that point to freed nodes (not in the live AST). diff --git a/src/V3Param.cpp b/src/V3Param.cpp index 9ca2b66ba..af9781dff 100644 --- a/src/V3Param.cpp +++ b/src/V3Param.cpp @@ -872,9 +872,7 @@ class ParamProcessor final { // Fix cross-module REFDTYPE pointers in newModp after cloneTree. // Phase A: path-based fixup using ledger entries with cellPath. - // Phase B: reachable-set fallback for remaining REFDTYPEs. - void fixupCrossModuleRefDTypes(AstNodeModule* newModp, AstNodeModule* srcModp, - AstNode* /*ifErrorp*/, const IfaceRefRefs& ifaceRefRefs) { + void fixupCrossModuleRefDTypes(AstNodeModule* newModp, AstNodeModule* srcModp) { if (!V3LinkDotIfaceCapture::enabled()) return; // Phase A: path-based fixup using ledger entries std::set ledgerFixed; @@ -923,51 +921,6 @@ class ParamProcessor final { }); V3Stats::addStatSum("IfaceCapture, Ledger fixups in V3Param", ledgerFixed.size()); } - - // Phase B: reachable-set fallback for REFDTYPEs not handled by ledger - std::set reachable; - reachable.insert(newModp); - std::function collectReachable; - collectReachable = [&](AstNodeModule* modp) { - for (AstNode* sp = modp->stmtsp(); sp; sp = sp->nextp()) { - if (AstCell* const cellp = VN_CAST(sp, Cell)) { - AstNodeModule* const cellModp = cellp->modp(); - if (cellModp && reachable.insert(cellModp).second) { - collectReachable(cellModp); - } - } - } - }; - for (const auto& pair : ifaceRefRefs) { - AstIface* const pinIfacep = pair.second->ifaceViaCellp(); - if (pinIfacep && reachable.insert(pinIfacep).second) { collectReachable(pinIfacep); } - } - collectReachable(newModp); - - // Phase B (reachable-set fallback): Phase A (path-based ledger fixup) - // always resolves all statement-level REFDTYPEs for current tests and - // Aerial. Assert if any REFDTYPE slips through so we can investigate. - // The loop body is assert-only (no mutations); LCOV_EXCL because - // Phase A always resolves everything and ledgerFixed catches all refs. - for (AstNode* stmtp = newModp->stmtsp(); stmtp; stmtp = stmtp->nextp()) { - AstRefDType* const refp = VN_CAST(stmtp, RefDType); - if (!refp) continue; - if (ledgerFixed.count(refp)) continue; // LCOV_EXCL_LINE - // LCOV_EXCL_START - // Check if typedefp or refDTypep points outside the reachable set - auto checkNotStale = [&](const char* label, AstNode* targetp) { - AstNodeModule* const ownerp = V3LinkDotIfaceCapture::findOwnerModule(targetp); - if (!ownerp || ownerp == newModp || VN_IS(ownerp, Package) - || reachable.count(ownerp)) - return; // OK: owner is reachable or self - v3fatalSrc("Phase B reachable-set fallback triggered for " - << refp->prettyNameQ() << " " << label << " owner=" - << ownerp->prettyNameQ() << " in " << newModp->prettyNameQ()); - }; - if (refp->typedefp()) checkNotStale("typedefp", refp->typedefp()); - if (refp->refDTypep()) checkNotStale("refDTypep", refp->refDTypep()); - // LCOV_EXCL_STOP - } } // Return true on success, false on error @@ -1155,10 +1108,9 @@ class ParamProcessor final { // to find the correct interface for each VarXRef. if (!ifaceRefRefs.empty()) { VarXRefRelinkVisitor{newModp}; } - // Fix cross-module REFDTYPE pointers in newModp (Phase A path-based - // + Phase B reachable-set fallback). + // Fix cross-module REFDTYPE pointers in newModp. UASSERT_OBJ(newModp, srcModp, "newModp null before hierarchy fixup"); - fixupCrossModuleRefDTypes(newModp, srcModp, ifErrorp, ifaceRefRefs); + fixupCrossModuleRefDTypes(newModp, srcModp); // Assign parameters to the constants specified // DOES clone() so must be finished with module clonep() before here diff --git a/test_regress/t/t_iface_capture_debug.py b/test_regress/t/t_iface_capture_debug.py new file mode 100755 index 000000000..b9e6450b6 --- /dev/null +++ b/test_regress/t/t_iface_capture_debug.py @@ -0,0 +1,21 @@ +#!/usr/bin/env python3 +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# This program is free software; you can redistribute it and/or modify it +# under the terms of either the GNU Lesser General Public License Version 3 +# or the Perl Artistic License Version 2.0. +# SPDX-FileCopyrightText: 2026 Wilson Snyder +# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +import vltest_bootstrap + +test.scenarios('vlt') +test.top_filename = "t/t_iface_typedef_bits_uaf.v" + +test.lint( + # Check we can dump the interface capture ledger + v_flags=["--debug --debugi 0 --debugi-V3LinkDotIfaceCapture 9"]) + +test.file_grep(test.compile_log_filename, r'iface capture dumpEntries: after finalizeIfaceCapture') + +test.passes()