From 567fba369531ed1ef6bd758f3c901cdd459ccb4f Mon Sep 17 00:00:00 2001 From: Cameron Waite Date: Sat, 24 Jan 2026 14:00:43 +1100 Subject: [PATCH] Fix null pointer dereference in class member trigger expressions (#6946) --- docs/CONTRIBUTORS | 1 + src/V3SenExprBuilder.h | 127 +++++++++++++++++++---- test_regress/t/t_class_trigger_null.py | 18 ++++ test_regress/t/t_class_trigger_null.v | 136 +++++++++++++++++++++++++ 4 files changed, 262 insertions(+), 20 deletions(-) create mode 100644 test_regress/t/t_class_trigger_null.py create mode 100644 test_regress/t/t_class_trigger_null.v diff --git a/docs/CONTRIBUTORS b/docs/CONTRIBUTORS index b01b9b33b..94e40d384 100644 --- a/docs/CONTRIBUTORS +++ b/docs/CONTRIBUTORS @@ -29,6 +29,7 @@ Aylon Chaim Porat Bartłomiej Chmiel Brian Li Cameron Kirk +Cameron Waite Chih-Mao Chen Chris Bachhuber Chris Randall diff --git a/src/V3SenExprBuilder.h b/src/V3SenExprBuilder.h index 2bf7ec432..a9d1dcd8b 100644 --- a/src/V3SenExprBuilder.h +++ b/src/V3SenExprBuilder.h @@ -72,6 +72,42 @@ private: }); } + // Check if expression contains a class member access that could be null + // (e.g., accessing an event through a class reference that may not be initialized) + static bool hasClassMemberAccess(const AstNode* const exprp) { + return exprp->exists([](const AstNode* const nodep) { + if (const AstMemberSel* const mselp = VN_CAST(nodep, MemberSel)) { + // Check if the base expression is a class reference + return mselp->fromp()->dtypep() + && VN_IS(mselp->fromp()->dtypep()->skipRefp(), ClassRefDType); + } + return false; + }); + } + + // Get the base class reference expression from a member selection chain + // Returns the outermost class reference that needs to be null-checked + // Note: Returns a pointer into the original tree - caller must clone if needed + static const AstNodeExpr* getBaseClassRef(const AstNodeExpr* exprp) { + while (exprp) { + if (const AstMemberSel* const mselp = VN_CAST(exprp, MemberSel)) { + const AstNodeExpr* const fromp = mselp->fromp(); + if (fromp->dtypep() && VN_IS(fromp->dtypep()->skipRefp(), ClassRefDType)) { + // Check if the base itself has class member access + if (hasClassMemberAccess(fromp)) { + exprp = fromp; + continue; + } + return fromp; + } + exprp = fromp; + } else { + return nullptr; + } + } + return nullptr; + } + // METHODS AstVarScope* crateTemp(AstNodeExpr* exprp) { // For readability, use the scoped signal name if the trigger is a simple AstVarRef @@ -89,6 +125,32 @@ private: return vscp; } + // Helper to wrap a statement with a null check: if (baseRef != null) stmt + AstNodeStmt* wrapStmtWithNullCheck(FileLine* flp, AstNodeStmt* stmtp, + const AstNodeExpr* baseClassRefp) { + if (!baseClassRefp) return stmtp; + AstNodeExpr* const nullp = new AstConst{flp, AstConst::Null{}}; + // const_cast safe: cloneTree doesn't modify the source + AstNodeExpr* const checkp = new AstNeq{ + flp, const_cast(baseClassRefp)->cloneTree(false), nullp}; + return new AstIf{flp, checkp, stmtp}; + } + + // Helper to wrap a trigger expression with a null check if needed + // Returns the expression wrapped in: (baseRef != null) ? expr : 0 + AstNodeExpr* wrapExprWithNullCheck(FileLine* flp, AstNodeExpr* exprp, + const AstNodeExpr* baseClassRefp) { + if (!baseClassRefp) return exprp; + AstNodeExpr* const nullp = new AstConst{flp, AstConst::Null{}}; + // const_cast safe: cloneTree doesn't modify the source + AstNodeExpr* const checkp = new AstNeq{ + flp, const_cast(baseClassRefp)->cloneTree(false), nullp}; + AstNodeExpr* const falsep = new AstConst{flp, AstConst::BitFalse{}}; + AstNodeExpr* const condp = new AstCond{flp, checkp, exprp, falsep}; + condp->dtypeSetBit(); + return condp; + } + AstNodeExpr* getCurr(AstNodeExpr* exprp) { // For simple expressions like varrefs or selects, just use them directly if (isSimpleExpr(exprp)) return exprp->cloneTree(false); @@ -99,17 +161,29 @@ private: if (result.second) result.first->second = crateTemp(exprp); AstVarScope* const currp = result.first->second; + // Check if we need null guards for class member access + const AstNodeExpr* const baseClassRefp + = hasClassMemberAccess(exprp) ? getBaseClassRef(exprp) : nullptr; + // Add pre update if it does not exist yet in this round if (m_hasPreUpdate.emplace(*currp).second) { - m_results.m_preUpdates.push_back(new AstAssign{ - flp, new AstVarRef{flp, currp, VAccess::WRITE}, exprp->cloneTree(false)}); + m_results.m_preUpdates.push_back( + wrapStmtWithNullCheck(flp, + new AstAssign{flp, new AstVarRef{flp, currp, VAccess::WRITE}, + exprp->cloneTree(false)}, + baseClassRefp)); } return new AstVarRef{flp, currp, VAccess::READ}; } + AstVarScope* getPrev(AstNodeExpr* exprp) { FileLine* const flp = exprp->fileline(); const auto rdCurr = [this, exprp]() { return getCurr(exprp); }; + // Check if we need null guards for class member access + const AstNodeExpr* const baseClassRefp + = hasClassMemberAccess(exprp) ? getBaseClassRef(exprp) : nullptr; + AstNode* scopeExprp = exprp; if (AstVarRef* const refp = VN_CAST(exprp, VarRef)) scopeExprp = refp->varScopep(); // Create the 'previous value' variable @@ -117,10 +191,10 @@ private: if (pair.second) { AstVarScope* const prevp = crateTemp(exprp); pair.first->second = prevp; - // Add the initializer init + // Add the initializer init (guarded if class member access) AstAssign* const initp = new AstAssign{flp, new AstVarRef{flp, prevp, VAccess::WRITE}, exprp->cloneTree(false)}; - m_results.m_inits.push_back(initp); + m_results.m_inits.push_back(wrapStmtWithNullCheck(flp, initp, baseClassRefp)); } AstVarScope* const prevp = pair.first->second; @@ -143,9 +217,12 @@ private: AstCMethodHard* const cmhp = new AstCMethodHard{flp, wrPrev(), VCMethod::UNPACKED_ASSIGN, rdCurr()}; cmhp->dtypeSetVoid(); - m_results.m_postUpdates.push_back(cmhp->makeStmt()); + m_results.m_postUpdates.push_back( + wrapStmtWithNullCheck(flp, cmhp->makeStmt(), baseClassRefp)); } else { - m_results.m_postUpdates.push_back(new AstAssign{flp, wrPrev(), rdCurr()}); + m_results.m_postUpdates.push_back( + wrapStmtWithNullCheck(flp, new AstAssign{flp, wrPrev(), rdCurr()}, + baseClassRefp)); } } @@ -156,6 +233,12 @@ private: FileLine* const flp = senItemp->fileline(); AstNodeExpr* const senp = senItemp->sensp(); + // Check if the sensitivity expression involves accessing through a class reference + // that may be null (e.g., DynScope handles created in fork blocks, or class member + // virtual interfaces). If so, we need to guard against null pointer dereference. + const AstNodeExpr* const baseClassRefp + = hasClassMemberAccess(senp) ? getBaseClassRef(senp) : nullptr; + const auto currp = [this, senp]() { return getCurr(senp); }; const auto prevp = [this, flp, senp]() { return new AstVarRef{flp, getPrev(senp), VAccess::READ}; }; @@ -171,31 +254,35 @@ private: AstCMethodHard* const resultp = new AstCMethodHard{flp, prevp(), VCMethod::UNPACKED_NEQ, currp()}; resultp->dtypeSetBit(); - return {resultp, true}; + return {wrapExprWithNullCheck(flp, resultp, baseClassRefp), true}; } - return {new AstNeq{flp, currp(), prevp()}, true}; + return {wrapExprWithNullCheck(flp, new AstNeq{flp, currp(), prevp()}, baseClassRefp), true}; case VEdgeType::ET_BOTHEDGE: // - return {lsb(new AstXor{flp, currp(), prevp()}), false}; + return {wrapExprWithNullCheck(flp, lsb(new AstXor{flp, currp(), prevp()}), baseClassRefp), + false}; case VEdgeType::ET_POSEDGE: // - return {lsb(new AstAnd{flp, currp(), new AstNot{flp, prevp()}}), false}; + return {wrapExprWithNullCheck(flp, lsb(new AstAnd{flp, currp(), new AstNot{flp, prevp()}}), + baseClassRefp), + false}; case VEdgeType::ET_NEGEDGE: // - return {lsb(new AstAnd{flp, new AstNot{flp, currp()}, prevp()}), false}; + return {wrapExprWithNullCheck(flp, lsb(new AstAnd{flp, new AstNot{flp, currp()}, prevp()}), + baseClassRefp), + false}; case VEdgeType::ET_EVENT: { UASSERT_OBJ(v3Global.hasEvents(), senItemp, "Inconsistent"); - { - // Clear 'fired' state when done - // No need to check if the event was fired, we need the flag clear regardless - AstCMethodHard* const clearp - = new AstCMethodHard{flp, currp(), VCMethod::EVENT_CLEAR_FIRED}; - clearp->dtypeSetVoid(); - m_results.m_postUpdates.push_back(clearp->makeStmt()); - } + + // Clear 'fired' state when done (guarded if class member access) + AstCMethodHard* const clearp + = new AstCMethodHard{flp, currp(), VCMethod::EVENT_CLEAR_FIRED}; + clearp->dtypeSetVoid(); + m_results.m_postUpdates.push_back( + wrapStmtWithNullCheck(flp, clearp->makeStmt(), baseClassRefp)); // Get 'fired' state AstCMethodHard* const callp = new AstCMethodHard{flp, currp(), VCMethod::EVENT_IS_FIRED}; callp->dtypeSetBit(); - return {callp, false}; + return {wrapExprWithNullCheck(flp, callp, baseClassRefp), false}; } case VEdgeType::ET_TRUE: // return {currp(), false}; diff --git a/test_regress/t/t_class_trigger_null.py b/test_regress/t/t_class_trigger_null.py new file mode 100644 index 000000000..bd059b0f2 --- /dev/null +++ b/test_regress/t/t_class_trigger_null.py @@ -0,0 +1,18 @@ +#!/usr/bin/env python3 +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2025 by Wilson Snyder. 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-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +import vltest_bootstrap + +test.scenarios('simulator') + +test.compile(verilator_flags2=['--binary']) + +test.execute() + +test.passes() diff --git a/test_regress/t/t_class_trigger_null.v b/test_regress/t/t_class_trigger_null.v new file mode 100644 index 000000000..4288a8761 --- /dev/null +++ b/test_regress/t/t_class_trigger_null.v @@ -0,0 +1,136 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2026 by Cameron Waite. +// SPDX-License-Identifier: CC0-1.0 + +// Test for null pointer dereference when sensitivity expressions reference +// class members. Verilator evaluates trigger expressions at simulation start, +// even for tasks that haven't been called yet. This caused null pointer +// dereference when accessing class_handle.event or class_handle.vif.signal +// because the task parameter didn't have a valid value yet. +// +// Also tests "instant fall-through" scenario: events triggered at time 0 +// (same timestep as class construction) should still be detected. + +interface my_if; + logic sig; +endinterface + +package my_pkg; + + class my_driver; + virtual my_if vif; + event my_event; + + function new(virtual my_if vif_in); + vif = vif_in; + endfunction + endclass + + // Task with sensitivity expressions on class member event and signals + task automatic my_task(my_driver drv, output int ev_cnt, pos_cnt, neg_cnt); + my_driver h = drv; + + if (h == null) begin + $display("Error: drv is NULL!"); + $finish; + end + + fork + begin + // Wait on class member event - previously caused null deref + @(h.my_event); + ev_cnt++; + end + begin + // Wait on posedge through virtual interface - previously caused null deref + @(posedge h.vif.sig); + pos_cnt++; + end + begin + // Wait on negedge through virtual interface - previously caused null deref + @(negedge h.vif.sig); + neg_cnt++; + end + begin + #10; + -> h.my_event; + #10; + h.vif.sig = 1; + #10; + h.vif.sig = 0; + end + join + endtask + +endpackage + +module t; + my_if intf(); + my_pkg::my_driver drv; + virtual my_if vif; + + int event_count; + int posedge_count; + int negedge_count; + + // Counter for instant (time 0) event detection + int instant_event_count = 0; + + // Test "instant fall-through": sensitivity on class member event at module level + // This always block is evaluated at elaboration when drv is still null. + // After construction, events triggered at time 0 should still be detected. + always @(drv.my_event) begin + instant_event_count++; + end + + // Construct the class at time 0 + initial begin + vif = intf; + drv = new(vif); + intf.sig = 0; + end + + // Trigger event at time 0 (same timestep as construction) - "instant fall-through" + // Use #0 to ensure construction happens first in delta cycle ordering + initial begin + #0; + -> drv.my_event; + end + + // Call the task later - trigger expressions were evaluated before this + initial begin + event_count = 0; + posedge_count = 0; + negedge_count = 0; + + #20; + my_pkg::my_task(drv, event_count, posedge_count, negedge_count); + + // Verify all triggers occurred + if (event_count != 1) begin + $display("%%Error: event_count = %0d, expected 1", event_count); + $stop; + end + if (posedge_count != 1) begin + $display("%%Error: posedge_count = %0d, expected 1", posedge_count); + $stop; + end + if (negedge_count != 1) begin + $display("%%Error: negedge_count = %0d, expected 1", negedge_count); + $stop; + end + + // Verify instant fall-through worked (time 0 event was detected) + // We expect 2: one from time 0, one from the task at time 30 + if (instant_event_count != 2) begin + $display("%%Error: instant_event_count = %0d, expected 2", instant_event_count); + $display("%%Error: Instant fall-through failed - events at time 0 were missed!"); + $stop; + end + + $write("*-* All Finished *-*\n"); + $finish; + end +endmodule