Gabriel B. has uploaded this change for review. (
https://gem5-review.googlesource.com/c/public/gem5/+/67452?usp=email )
Change subject: mem-ruby: Fix nullptr deref with base Port classes
......................................................................
mem-ruby: Fix nullptr deref with base Port classes
Change-Id: I29214278c3dd4829c89a6f7c93214b8123912e74
---
M src/arch/arm/table_walker.cc
M src/cpu/simple/atomic.cc
M src/mem/cache/base.cc
M src/mem/port.cc
M src/mem/port.hh
M src/mem/ruby/system/RubyPort.cc
M src/sim/port.cc
M src/sim/port.hh
8 files changed, 106 insertions(+), 64 deletions(-)
diff --git a/src/arch/arm/table_walker.cc b/src/arch/arm/table_walker.cc
index bbf102d..8e16e9b 100644
--- a/src/arch/arm/table_walker.cc
+++ b/src/arch/arm/table_walker.cc
@@ -234,7 +234,7 @@
Addr size, Tick delay)
{
if (state->event) {
- owner.schedule(state->event, curTick() + delay);
+ owner().schedule(state->event, curTick() + delay);
}
delete state;
}
diff --git a/src/cpu/simple/atomic.cc b/src/cpu/simple/atomic.cc
index d6638b3..770bbf4 100644
--- a/src/cpu/simple/atomic.cc
+++ b/src/cpu/simple/atomic.cc
@@ -281,7 +281,7 @@
__func__, pkt->getAddr(), pkt->cmdString());
// X86 ISA: Snooping an invalidation for monitor/mwait
- AtomicSimpleCPU *cpu = (AtomicSimpleCPU *)(&owner);
+ AtomicSimpleCPU *cpu = (AtomicSimpleCPU *)(&owner());
for (ThreadID tid = 0; tid < cpu->numThreads; tid++) {
if (cpu->getCpuAddrMonitor(tid)->doMonitor(pkt)) {
@@ -312,7 +312,7 @@
__func__, pkt->getAddr(), pkt->cmdString());
// X86 ISA: Snooping an invalidation for monitor/mwait
- AtomicSimpleCPU *cpu = (AtomicSimpleCPU *)(&owner);
+ AtomicSimpleCPU *cpu = (AtomicSimpleCPU *)(&owner());
for (ThreadID tid = 0; tid < cpu->numThreads; tid++) {
if (cpu->getCpuAddrMonitor(tid)->doMonitor(pkt)) {
cpu->wakeup(tid);
diff --git a/src/mem/cache/base.cc b/src/mem/cache/base.cc
index cf6c9fe..cbf4bea 100644
--- a/src/mem/cache/base.cc
+++ b/src/mem/cache/base.cc
@@ -150,7 +150,7 @@
// if we already scheduled a retry in this cycle, but it has not yet
// happened, cancel it
if (sendRetryEvent.scheduled()) {
- owner.deschedule(sendRetryEvent);
+ owner().deschedule(sendRetryEvent);
DPRINTF(CachePort, "Port descheduled retry\n");
mustSendRetry = true;
}
@@ -164,7 +164,7 @@
blocked = false;
if (mustSendRetry) {
// @TODO: need to find a better time (next cycle?)
- owner.schedule(sendRetryEvent, curTick() + 1);
+ owner().schedule(sendRetryEvent, curTick() + 1);
}
}
diff --git a/src/mem/port.cc b/src/mem/port.cc
index 18793d4..316a76a 100644
--- a/src/mem/port.cc
+++ b/src/mem/port.cc
@@ -121,8 +121,7 @@
* Request port
*/
RequestPort::RequestPort(const std::string& name, SimObject* _owner,
- PortID _id) : Port(name, _id), _responsePort(&defaultResponsePort),
- owner(*_owner)
+ PortID _id) : Port(name, _owner, _id),
_responsePort(&defaultResponsePort)
{
}
@@ -176,8 +175,8 @@
* Response port
*/
ResponsePort::ResponsePort(const std::string& name, SimObject* _owner,
- PortID id) : Port(name, id), _requestPort(&defaultRequestPort),
- defaultBackdoorWarned(false), owner(*_owner)
+ PortID id) : Port(name, _owner, id), _requestPort(&defaultRequestPort),
+ defaultBackdoorWarned(false)
{
}
diff --git a/src/mem/port.hh b/src/mem/port.hh
index fb0f4b8..fe5a12a 100644
--- a/src/mem/port.hh
+++ b/src/mem/port.hh
@@ -82,9 +82,6 @@
private:
ResponsePort *_responsePort;
- protected:
- SimObject &owner;
-
public:
RequestPort(const std::string& name, SimObject* _owner,
PortID id=InvalidPortID);
@@ -290,9 +287,6 @@
bool defaultBackdoorWarned;
- protected:
- SimObject& owner;
-
public:
ResponsePort(const std::string& name, SimObject* _owner,
PortID id=InvalidPortID);
diff --git a/src/mem/ruby/system/RubyPort.cc
b/src/mem/ruby/system/RubyPort.cc
index 48f655d..28055e2 100644
--- a/src/mem/ruby/system/RubyPort.cc
+++ b/src/mem/ruby/system/RubyPort.cc
@@ -171,12 +171,12 @@
bool
RubyPort::PioRequestPort::recvTimingResp(PacketPtr pkt)
{
- RubyPort *rp = static_cast<RubyPort *>(&owner);
+ RubyPort& rp = safe_cast<RubyPort&>(owner());
DPRINTF(RubyPort, "Response for address: 0x%#x\n", pkt->getAddr());
// send next cycle
- rp->pioResponsePort.schedTimingResp(
- pkt, curTick() + rp->m_ruby_system->clockPeriod());
+ rp.pioResponsePort.schedTimingResp(
+ pkt, curTick() + rp.m_ruby_system->clockPeriod());
return true;
}
@@ -199,8 +199,8 @@
pkt->getAddr(), port->name());
// attempt to send the response in the next cycle
- RubyPort *rp = static_cast<RubyPort *>(&owner);
- port->schedTimingResp(pkt, curTick() +
rp->m_ruby_system->clockPeriod());
+ RubyPort& rp = safe_cast<RubyPort&>(owner());
+ port->schedTimingResp(pkt, curTick() +
rp.m_ruby_system->clockPeriod());
return true;
}
@@ -208,16 +208,16 @@
bool
RubyPort::PioResponsePort::recvTimingReq(PacketPtr pkt)
{
- RubyPort *ruby_port = static_cast<RubyPort *>(&owner);
+ auto& ruby_port = safe_cast<RubyPort&>(owner());
- for (size_t i = 0; i < ruby_port->request_ports.size(); ++i) {
- AddrRangeList l = ruby_port->request_ports[i]->getAddrRanges();
+ for (size_t i = 0; i < ruby_port.request_ports.size(); ++i) {
+ AddrRangeList l = ruby_port.request_ports[i]->getAddrRanges();
for (auto it = l.begin(); it != l.end(); ++it) {
if (it->contains(pkt->getAddr())) {
// generally it is not safe to assume success here as
// the port could be blocked
[[maybe_unused]] bool success =
- ruby_port->request_ports[i]->sendTimingReq(pkt);
+ ruby_port.request_ports[i]->sendTimingReq(pkt);
assert(success);
return true;
}
@@ -229,17 +229,17 @@
Tick
RubyPort::PioResponsePort::recvAtomic(PacketPtr pkt)
{
- RubyPort *ruby_port = static_cast<RubyPort *>(&owner);
+ auto& ruby_port = safe_cast<RubyPort&>(owner());
// Only atomic_noncaching mode supported!
- if (!ruby_port->system->bypassCaches()) {
+ if (!ruby_port.system->bypassCaches()) {
panic("Ruby supports atomic accesses only in noncaching mode\n");
}
- for (size_t i = 0; i < ruby_port->request_ports.size(); ++i) {
- AddrRangeList l = ruby_port->request_ports[i]->getAddrRanges();
+ for (size_t i = 0; i < ruby_port.request_ports.size(); ++i) {
+ AddrRangeList l = ruby_port.request_ports[i]->getAddrRanges();
for (auto it = l.begin(); it != l.end(); ++it) {
if (it->contains(pkt->getAddr())) {
- return ruby_port->request_ports[i]->sendAtomic(pkt);
+ return ruby_port.request_ports[i]->sendAtomic(pkt);
}
}
}
@@ -251,7 +251,7 @@
{
DPRINTF(RubyPort, "Timing request for address %#x on port %d\n",
pkt->getAddr(), id);
- RubyPort *ruby_port = static_cast<RubyPort *>(&owner);
+ auto& ruby_port = safe_cast<RubyPort&>(owner());
if (pkt->cacheResponding())
panic("RubyPort should never see request with the "
@@ -269,7 +269,7 @@
// pio port.
if (pkt->cmd != MemCmd::MemSyncReq) {
if (!pkt->req->isMemMgmt() && !isPhysMemAddress(pkt)) {
- assert(ruby_port->memRequestPort.isConnected());
+ assert(ruby_port.memRequestPort.isConnected());
DPRINTF(RubyPort, "Request address %#x assumed to be a "
"pio address\n", pkt->getAddr());
@@ -278,8 +278,8 @@
pkt->pushSenderState(new SenderState(this));
// send next cycle
- RubySystem *rs = ruby_port->m_ruby_system;
- ruby_port->memRequestPort.schedTimingReq(pkt,
+ RubySystem *rs = ruby_port.m_ruby_system;
+ ruby_port.memRequestPort.schedTimingReq(pkt,
curTick() + rs->clockPeriod());
return true;
}
@@ -290,7 +290,7 @@
pkt->pushSenderState(new SenderState(this));
// Submit the ruby request
- RequestStatus requestStatus = ruby_port->makeRequest(pkt);
+ RequestStatus requestStatus = ruby_port.makeRequest(pkt);
// If the request successfully issued then we should return true.
// Otherwise, we need to tell the port to retry at a later point
@@ -320,9 +320,9 @@
Tick
RubyPort::MemResponsePort::recvAtomic(PacketPtr pkt)
{
- RubyPort *ruby_port = static_cast<RubyPort *>(&owner);
+ auto& ruby_port = safe_cast<RubyPort&>(owner());
// Only atomic_noncaching mode supported!
- if (!ruby_port->system->bypassCaches()) {
+ if (!ruby_port.system->bypassCaches()) {
panic("Ruby supports atomic accesses only in noncaching mode\n");
}
@@ -330,7 +330,7 @@
// pio port.
if (pkt->cmd != MemCmd::MemSyncReq) {
if (!isPhysMemAddress(pkt)) {
- assert(ruby_port->memRequestPort.isConnected());
+ assert(ruby_port.memRequestPort.isConnected());
DPRINTF(RubyPort, "Request address %#x assumed to be a "
"pio address\n", pkt->getAddr());
@@ -339,8 +339,8 @@
pkt->pushSenderState(new SenderState(this));
// send next cycle
- Tick req_ticks = ruby_port->memRequestPort.sendAtomic(pkt);
- return ruby_port->ticksToCycles(req_ticks);
+ Tick req_ticks = ruby_port.memRequestPort.sendAtomic(pkt);
+ return ruby_port.ticksToCycles(req_ticks);
}
assert(getOffset(pkt->getAddr()) + pkt->getSize() <=
@@ -348,7 +348,7 @@
}
// Find the machine type of memory controller interface
- RubySystem *rs = ruby_port->m_ruby_system;
+ RubySystem *rs = ruby_port.m_ruby_system;
static int mem_interface_type = -1;
if (mem_interface_type == -1) {
if (rs->m_abstract_controls[MachineType_Directory].size() != 0) {
@@ -363,7 +363,7 @@
}
// Find the controller for the target address
- MachineID id = ruby_port->m_controller->mapAddressToMachine(
+ MachineID id = ruby_port.m_controller->mapAddressToMachine(
pkt->getAddr(), (MachineType)mem_interface_type);
AbstractController *mem_interface =
rs->m_abstract_controls[mem_interface_type][id.getNum()];
@@ -376,15 +376,15 @@
void
RubyPort::MemResponsePort::addToRetryList()
{
- RubyPort *ruby_port = static_cast<RubyPort *>(&owner);
+ auto& ruby_port = safe_cast<RubyPort&>(owner());
//
// Unless the request port do not want retries (e.g., the Ruby tester),
// record the stalled M5 port for later retry when the sequencer
// becomes free.
//
- if (!no_retry_on_stall && !ruby_port->onRetryList(this)) {
- ruby_port->addToRetryList(this);
+ if (!no_retry_on_stall && !ruby_port.onRetryList(this)) {
+ ruby_port.addToRetryList(this);
}
}
@@ -393,15 +393,15 @@
{
DPRINTF(RubyPort, "Functional access for address: %#x\n",
pkt->getAddr());
- [[maybe_unused]] RubyPort *rp = static_cast<RubyPort *>(&owner);
- RubySystem *rs = rp->m_ruby_system;
+ [[maybe_unused]] RubyPort& rp = safe_cast<RubyPort&>(owner());
+ RubySystem *rs = rp.m_ruby_system;
// Check for pio requests and directly send them to the dedicated
// pio port.
if (!isPhysMemAddress(pkt)) {
DPRINTF(RubyPort, "Pio Request for address: 0x%#x\n",
pkt->getAddr());
- assert(rp->pioRequestPort.isConnected());
- rp->pioRequestPort.sendFunctional(pkt);
+ assert(rp.pioRequestPort.isConnected());
+ rp.pioRequestPort.sendFunctional(pkt);
return;
}
@@ -626,15 +626,15 @@
DPRINTF(RubyPort, "Hit callback needs response %d\n", needsResponse);
- RubyPort *ruby_port = static_cast<RubyPort *>(&owner);
- RubySystem *rs = ruby_port->m_ruby_system;
+ auto& ruby_port = safe_cast<RubyPort&>(owner());
+ RubySystem *rs = ruby_port.m_ruby_system;
if (accessPhysMem) {
// We must check device memory first in case it overlaps with the
// system memory range.
- if (ruby_port->system->isDeviceMemAddr(pkt)) {
- auto dmem = ruby_port->system->getDeviceMemory(pkt);
+ if (ruby_port.system->isDeviceMemAddr(pkt)) {
+ auto dmem = ruby_port.system->getDeviceMemory(pkt);
dmem->access(pkt);
- } else if (ruby_port->system->isMemAddr(pkt->getAddr())) {
+ } else if (ruby_port.system->isMemAddr(pkt->getAddr())) {
rs->getPhysMem()->access(pkt);
} else {
panic("Packet is in neither device nor system memory!");
@@ -662,11 +662,11 @@
{
// at the moment the assumption is that the request port does not care
AddrRangeList ranges;
- RubyPort *ruby_port = static_cast<RubyPort *>(&owner);
+ auto& ruby_port = safe_cast<RubyPort const&>(owner());
- for (size_t i = 0; i < ruby_port->request_ports.size(); ++i) {
+ for (size_t i = 0; i < ruby_port.request_ports.size(); ++i) {
ranges.splice(ranges.begin(),
- ruby_port->request_ports[i]->getAddrRanges());
+ ruby_port.request_ports[i]->getAddrRanges());
}
for ([[maybe_unused]] const auto &r : ranges)
DPRINTF(RubyPort, "%s\n", r.to_string());
@@ -676,8 +676,8 @@
bool
RubyPort::MemResponsePort::isShadowRomAddress(Addr addr) const
{
- RubyPort *ruby_port = static_cast<RubyPort *>(&owner);
- AddrRangeList ranges = ruby_port->system->getShadowRomRanges();
+ auto& ruby_port = safe_cast<RubyPort const&>(owner());
+ AddrRangeList ranges = ruby_port.system->getShadowRomRanges();
for (auto it = ranges.begin(); it != ranges.end(); ++it) {
if (it->contains(addr)) {
@@ -691,10 +691,10 @@
bool
RubyPort::MemResponsePort::isPhysMemAddress(PacketPtr pkt) const
{
- RubyPort *ruby_port = static_cast<RubyPort *>(&owner);
+ auto& ruby_port = safe_cast<RubyPort const&>(owner());
Addr addr = pkt->getAddr();
- return (ruby_port->system->isMemAddr(addr)
&& !isShadowRomAddress(addr))
- || ruby_port->system->isDeviceMemAddr(pkt);
+ return (ruby_port.system->isMemAddr(addr) && !isShadowRomAddress(addr))
+ || ruby_port.system->isDeviceMemAddr(pkt);
}
void
@@ -724,7 +724,7 @@
void
RubyPort::PioRequestPort::recvRangeChange()
{
- RubyPort &r = static_cast<RubyPort &>(owner);
+ RubyPort &r = static_cast<RubyPort &>(owner());
r.gotAddrRanges--;
if (r.gotAddrRanges == 0 && FullSystem) {
r.pioResponsePort.sendRangeChange();
diff --git a/src/sim/port.cc b/src/sim/port.cc
index 580469a..37ffe72 100644
--- a/src/sim/port.cc
+++ b/src/sim/port.cc
@@ -50,11 +50,32 @@
namespace gem5
{
-Port::Port(const std::string& _name, PortID _id) :
- portName(_name), id(_id), _peer(nullptr), _connected(false)
+Port::Port(const std::string& _name, SimObject* owner, PortID _id) :
+ portName{_name},
+ portOwner{owner},
+ id{_id},
+ _peer{nullptr},
+ _connected{false}
{}
+
+Port::Port(const std::string& _name, PortID _id) :
+ Port{_name, nullptr, _id}
+{}
+
Port::~Port() {}
+SimObject&
+Port::owner()
+{
+ return const_cast<SimObject&>(const_cast<const Port*>(this)->owner());
+}
+
+SimObject const&
+Port::owner() const
+{
+ panic_if(!portOwner, "Requesting owner of ownerless port.");
+ return *portOwner;
+}
void
Port::reportUnbound() const
diff --git a/src/sim/port.hh b/src/sim/port.hh
index 4f4a163..2b5b759 100644
--- a/src/sim/port.hh
+++ b/src/sim/port.hh
@@ -55,6 +55,8 @@
namespace gem5
{
+class SimObject;
+
/**
* Ports are used to interface objects to each other.
*/
@@ -66,6 +68,8 @@
/** Descriptive name (for DPRINTF output) */
const std::string portName;
+ SimObject* portOwner;
+
protected:
class UnboundPortException {};
@@ -93,6 +97,15 @@
* Abstract base class for ports
*
* @param _name Port name including the owners name
+ * @param _owner Owner of the port
+ * @param _id A port identifier for vector ports
+ */
+ Port(const std::string& _name, SimObject* _owner, PortID _id);
+
+ /**
+ * Constructor for ports with no owner
+ *
+ * @param _name Port name including the owners name
* @param _id A port identifier for vector ports
*/
Port(const std::string& _name, PortID _id);
@@ -104,6 +117,12 @@
*/
virtual ~Port();
+ /**
+ * Return a reference to this port's owner
+ */
+ SimObject& owner();
+ SimObject const& owner() const;
+
/** Return a reference to this port's peer. */
Port &getPeer() { return *_peer; }
--
To view, visit
https://gem5-review.googlesource.com/c/public/gem5/+/67452?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gem5-review.googlesource.com/settings
Gerrit-Project: public/gem5
Gerrit-Branch: develop
Gerrit-Change-Id: I29214278c3dd4829c89a6f7c93214b8123912e74
Gerrit-Change-Number: 67452
Gerrit-PatchSet: 1
Gerrit-Owner: Gabriel B. <gabriel.bus...@arteris.com>
Gerrit-MessageType: newchange
_______________________________________________
gem5-dev mailing list -- gem5-dev@gem5.org
To unsubscribe send an email to gem5-dev-le...@gem5.org
