luke
/
verilator
mirror of https://github.com/verilator/verilator.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix null pointer dereference in class member trigger expressions (#6946)

This commit is contained in:
Cameron Waite 2026-01-24 14:00:43 +11:00 committed by GitHub
parent bc3c5b32dd
commit 567fba3695
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 262 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
docs/CONTRIBUTORS
View File

@ -29,6 +29,7 @@ Aylon Chaim Porat
Bartłomiej Chmiel
Brian Li
Cameron Kirk
Cameron Waite
Chih-Mao Chen
Chris Bachhuber
Chris Randall

127
src/V3SenExprBuilder.h
View File

@ -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<AstNodeExpr*>(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<AstNodeExpr*>(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};

18
test_regress/t/t_class_trigger_null.py Normal file
View File

@ -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()

136
test_regress/t/t_class_trigger_null.v Normal file
View File

@ -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