Skip to content

Commit

Permalink
[fdborch] fix heap-use-after-free in clearFdbEntry() (sonic-net#2353)
Browse files Browse the repository at this point in the history
- What I did
using a copy of FDBEntry fields (stored in FDBUpdate) instead of a reference since the reference gets invalidated in the storeFdbEntryState()
simplified clearFdbEntry() interface

- Why I did it
To fix the memory usage issue
The issue is that the SWSS_LOG_INFO() uses the mac&, port_alias&, and bv_id& which are invalidated in the storeFdbEntryState().

- How I verified it
Run the tests that were used to find the issues and checked the ASAN report

Signed-off-by: Yakiv Huryk <[email protected]>
  • Loading branch information
Yakiv-Huryk authored Jun 24, 2022
1 parent 1b8bd94 commit 84e9b07
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 13 deletions.
21 changes: 9 additions & 12 deletions orchagent/fdborch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -177,31 +177,28 @@ bool FdbOrch::storeFdbEntryState(const FdbUpdate& update)
/*
clears stateDb and decrements corresponding internal fdb counters
*/
void FdbOrch::clearFdbEntry(const MacAddress& mac,
const sai_object_id_t& bv_id,
const string& port_alias)
void FdbOrch::clearFdbEntry(const FdbEntry& entry)
{
FdbUpdate update;
update.entry.mac = mac;
update.entry.bv_id = bv_id;
update.entry = entry;
update.add = false;

/* Fetch Vlan and decrement the counter */
Port temp_vlan;
if (m_portsOrch->getPort(bv_id, temp_vlan))
if (m_portsOrch->getPort(entry.bv_id, temp_vlan))
{
m_portsOrch->decrFdbCount(temp_vlan.m_alias, 1);
}

/* Decrement port fdb_counter */
m_portsOrch->decrFdbCount(port_alias, 1);
m_portsOrch->decrFdbCount(entry.port_name, 1);

/* Remove the FdbEntry from the internal cache, update state DB and CRM counter */
storeFdbEntryState(update);
notify(SUBJECT_TYPE_FDB_CHANGE, &update);

SWSS_LOG_INFO("FdbEntry removed from internal cache, MAC: %s , port: %s, BVID: 0x%" PRIx64,
mac.to_string().c_str(), port_alias.c_str(), bv_id);
update.entry.mac.to_string().c_str(), update.entry.port_name.c_str(), update.entry.bv_id);
}

/*
Expand All @@ -224,7 +221,7 @@ void FdbOrch::handleSyncdFlushNotif(const sai_object_id_t& bv_id,
auto curr = itr++;
if (curr->second.type != "static" && (curr->first.mac == mac || mac == flush_mac))
{
clearFdbEntry(curr->first.mac, curr->first.bv_id, curr->first.port_name);
clearFdbEntry(curr->first);
}
}
}
Expand All @@ -238,7 +235,7 @@ void FdbOrch::handleSyncdFlushNotif(const sai_object_id_t& bv_id,
{
if (curr->second.type != "static" && (curr->first.mac == mac || mac == flush_mac))
{
clearFdbEntry(curr->first.mac, curr->first.bv_id, curr->first.port_name);
clearFdbEntry(curr->first);
}
}
}
Expand All @@ -253,7 +250,7 @@ void FdbOrch::handleSyncdFlushNotif(const sai_object_id_t& bv_id,
{
if (curr->second.type != "static" && (curr->first.mac == mac || mac == flush_mac))
{
clearFdbEntry(curr->first.mac, curr->first.bv_id, curr->first.port_name);
clearFdbEntry(curr->first);
}
}
}
Expand All @@ -268,7 +265,7 @@ void FdbOrch::handleSyncdFlushNotif(const sai_object_id_t& bv_id,
{
if (curr->second.type != "static" && (curr->first.mac == mac || mac == flush_mac))
{
clearFdbEntry(curr->first.mac, curr->first.bv_id, curr->first.port_name);
clearFdbEntry(curr->first);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion orchagent/fdborch.h
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ class FdbOrch: public Orch, public Subject, public Observer
bool storeFdbEntryState(const FdbUpdate& update);
void notifyTunnelOrch(Port& port);

void clearFdbEntry(const MacAddress&, const sai_object_id_t&, const string&);
void clearFdbEntry(const FdbEntry&);
void handleSyncdFlushNotif(const sai_object_id_t&, const sai_object_id_t&, const MacAddress& );
};

Expand Down

0 comments on commit 84e9b07

Please sign in to comment.