Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use smart pointers for PcapLiveDeviceList's internal memory management. #1440

Merged
merged 7 commits into from
Jun 8, 2024
Merged

Use smart pointers for PcapLiveDeviceList's internal memory management. #1440

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d78f143
Updated 'private deleted' constructors to cpp11 standard.
Dimi1010 Jun 6, 2024
f85ef1e
Changed PcapLiveDeviceList to hold its PcapLiveDevices in shared poin…
Dimi1010 Jun 6, 2024
f559505
Renamed variables in loops to be more clear.
Dimi1010 Jun 6, 2024
5e688f2
Moved view vector to be constructed directly in initialization.
Dimi1010 Jun 7, 2024
ad77379
Changed shared_ptr to unique_ptr
Dimi1010 Jun 7, 2024
f814c06
Merge remote-tracking branch 'upstream/dev' into feature/pcap-live-de…
Dimi1010 Jun 7, 2024
5822cbe
Merge remote-tracking branch 'upstream/dev' into feature/pcap-live-de…
Dimi1010 Jun 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions Pcap++/header/PcapLiveDeviceList.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include "IpAddress.h"
#include "PcapLiveDevice.h"
#include <vector>
#include <memory>


/// @file
Expand All @@ -23,20 +24,26 @@ namespace pcpp
class PcapLiveDeviceList
{
private:
std::vector<PcapLiveDevice*> m_LiveDeviceList;
std::vector<std::shared_ptr<PcapLiveDevice>> m_LiveDeviceList;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this a vector of shared_ptr and not unique_ptr? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation uses shared pointers to store the live devices in preparation for point 4 of #1431.

From the PR description. I plan on having the API return shared pointers.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I fully understand point 4, but I'm not sure there's a good reason to unify the APIs of PcapLiveDeviceList and PcapRemoteDeviceList, from multiple reasons:

  • PcapLiveDeviceList is a singleton because it finds all the interfaces on the machine. However PcapRemoteDeviceList is not a singleton because it encapsulates remote devices on other machines so a user can have multiple instances of it. The API is quite different so not sure it's a good idea to merge it
  • Remote devices are a niche that isn't used a lot, whereas pcap devices is probably the most popular feature of PcapPlusPlus. I wouldn't mix between them

Copy link
Collaborator Author

@Dimi1010 Dimi1010 Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, one of the reasons I started thinking about it is that both PcapLiveDeviceList and PcapRemoteDeviceList share a couple similarities and have some code duplication.

  • Both store their devices in vector<DevicePtr>
  • The methods getPcapLiveDeviceByIp and getPcapRemoteDeviceByIP have practically duplicate implementation.

One of the reasons for unifying the API is having an internal base class template<DeviceType> PcapLiveDeviceListBase (name subject to change) that implements the getPcapDeviceByIP methods that currently have duplicate implementation.

The current methods could still be kept and just call the new functions too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, even if the API is not unified, I still have the proposal to get the API to return shared_ptr instead of raw pointers so you don't suddenly get invalid pointers. (refresh, or in case of RemoteDeviceList just the list going out of scope).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be honest, I wouldn't spend time on it because although there is some code duplication, it's not a lot and we can keep the code simple. If you really insist we can discuss it more...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, ok then. Gonna shelve point 4 for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PcapLiveDeviceList is a singleton

@seladb Btw, if the class is a singleton, why does it have a clone method?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to unique_ptr. ad77379

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PcapLiveDeviceList is a singleton

@seladb Btw, if the class is a singleton, why does it have a clone method?

I agree this API is a bit weird 🙈
The reason is that users wanted to capture traffic from the same interface more than once, here is the GitHub issue: #766

The idea was to allow cloning a live device, or to allow cloning all devices...

// Vector of raw device pointers to keep the signature of getPcapLiveDevicesList, as it returns a reference.
mutable std::vector<PcapLiveDevice*> m_LiveDeviceListView;

std::vector<IPv4Address> m_DnsServers;

// private c'tor
PcapLiveDeviceList();
// private copy c'tor
PcapLiveDeviceList( const PcapLiveDeviceList& other );
PcapLiveDeviceList& operator=(const PcapLiveDeviceList& other);

void init();

void setDnsServers();

void updateLiveDeviceListView() const;
public:
PcapLiveDeviceList(const PcapLiveDeviceList&) = delete;
PcapLiveDeviceList(PcapLiveDeviceList&&) noexcept = delete;
PcapLiveDeviceList& operator=(const PcapLiveDeviceList&) = delete;
PcapLiveDeviceList& operator=(PcapLiveDeviceList&&) noexcept = delete;

/**
* The access method to the singleton
* @return The singleton instance of this class
Expand All @@ -50,7 +57,7 @@ namespace pcpp
/**
* @return A vector containing pointers to all live devices currently installed on the machine
*/
const std::vector<PcapLiveDevice*>& getPcapLiveDevicesList() const { return m_LiveDeviceList; }
const std::vector<PcapLiveDevice*>& getPcapLiveDevicesList() const;

/**
* Get a pointer to the live device by its IP address. IP address can be both IPv4 or IPv6
Expand Down Expand Up @@ -110,9 +117,6 @@ namespace pcpp
* Reset the live device list and DNS server list, meaning clear and refetch them
*/
void reset();

// d'tor
~PcapLiveDeviceList();
};

} // namespace pcpp
71 changes: 39 additions & 32 deletions Pcap++/src/PcapLiveDeviceList.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,6 @@ PcapLiveDeviceList::PcapLiveDeviceList()
init();
}

PcapLiveDeviceList::~PcapLiveDeviceList()
{
for(const auto &devIter : m_LiveDeviceList)
{
delete devIter;
}
}

void PcapLiveDeviceList::init()
{
pcap_if_t* interfaceList;
Expand All @@ -53,12 +45,12 @@ void PcapLiveDeviceList::init()
while (currInterface != nullptr)
{
#if defined(_WIN32)
PcapLiveDevice* dev = new WinPcapLiveDevice(currInterface, true, true, true);
auto dev = std::unique_ptr<PcapLiveDevice>(new WinPcapLiveDevice(currInterface, true, true, true));
#else //__linux__, __APPLE__, __FreeBSD__
PcapLiveDevice* dev = new PcapLiveDevice(currInterface, true, true, true);
auto dev = std::unique_ptr<PcapLiveDevice>(new PcapLiveDevice(currInterface, true, true, true));
#endif
currInterface = currInterface->next;
m_LiveDeviceList.insert(m_LiveDeviceList.end(), dev);
m_LiveDeviceList.push_back(std::move(dev));
}

setDnsServers();
Expand Down Expand Up @@ -253,6 +245,25 @@ void PcapLiveDeviceList::setDnsServers()
#endif
}

void PcapLiveDeviceList::updateLiveDeviceListView() const
{
// There is a potential issue if a device is removed and another one is added between updates,
// but as far as I can see the LiveDeviceList is never partially modified.
if (m_LiveDeviceList.size() != m_LiveDeviceListView.size())
Dimi1010 marked this conversation as resolved.
Show resolved Hide resolved
{
m_LiveDeviceListView.resize(m_LiveDeviceList.size());
// Full update of all elements of the view vector to synchronize them with the main vector.
std::transform(m_LiveDeviceList.begin(), m_LiveDeviceList.end(), m_LiveDeviceListView.begin(),
[](const std::shared_ptr<PcapLiveDevice>& ptr) { return ptr.get(); });
}
}

const std::vector<PcapLiveDevice*>& PcapLiveDeviceList::getPcapLiveDevicesList() const
{
updateLiveDeviceListView();
return m_LiveDeviceListView;
}

PcapLiveDevice* PcapLiveDeviceList::getPcapLiveDeviceByIp(const IPAddress& ipAddr) const
{
if (ipAddr.getType() == IPAddress::IPv4AddressType)
Expand All @@ -268,19 +279,19 @@ PcapLiveDevice* PcapLiveDeviceList::getPcapLiveDeviceByIp(const IPAddress& ipAdd
PcapLiveDevice* PcapLiveDeviceList::getPcapLiveDeviceByIp(const IPv4Address& ipAddr) const
{
PCPP_LOG_DEBUG("Searching all live devices...");
for(const auto &devIter : m_LiveDeviceList)
for(const auto& devicePtr : m_LiveDeviceList)
{
PCPP_LOG_DEBUG("Searching device '" << devIter->m_Name << "'. Searching all addresses...");
for(const auto &addrIter : devIter->m_Addresses)
PCPP_LOG_DEBUG("Searching device '" << devicePtr->m_Name << "'. Searching all addresses...");
for(const auto& addressInfo : devicePtr->m_Addresses)
{
if (Logger::getInstance().isDebugEnabled(PcapLogModuleLiveDevice) && addrIter.addr != nullptr)
if (Logger::getInstance().isDebugEnabled(PcapLogModuleLiveDevice) && addressInfo.addr != nullptr)
{
std::array<char, INET6_ADDRSTRLEN> addrAsString;
internal::sockaddr2string(addrIter.addr, addrAsString.data(), addrAsString.size());
internal::sockaddr2string(addressInfo.addr, addrAsString.data(), addrAsString.size());
PCPP_LOG_DEBUG("Searching address " << addrAsString.data());
}

in_addr* currAddr = internal::try_sockaddr2in_addr(addrIter.addr);
in_addr* currAddr = internal::try_sockaddr2in_addr(addressInfo.addr);
if (currAddr == nullptr)
{
PCPP_LOG_DEBUG("Address is nullptr");
Expand All @@ -290,7 +301,7 @@ PcapLiveDevice* PcapLiveDeviceList::getPcapLiveDeviceByIp(const IPv4Address& ipA
if (*currAddr == ipAddr)
{
PCPP_LOG_DEBUG("Found matched address!");
return devIter;
return devicePtr.get();
}
}
}
Expand All @@ -301,19 +312,19 @@ PcapLiveDevice* PcapLiveDeviceList::getPcapLiveDeviceByIp(const IPv4Address& ipA
PcapLiveDevice* PcapLiveDeviceList::getPcapLiveDeviceByIp(const IPv6Address& ip6Addr) const
{
PCPP_LOG_DEBUG("Searching all live devices...");
for(const auto &devIter : m_LiveDeviceList)
for(const auto& devicePtr : m_LiveDeviceList)
{
PCPP_LOG_DEBUG("Searching device '" << devIter->m_Name << "'. Searching all addresses...");
for(const auto &addrIter : devIter->m_Addresses)
PCPP_LOG_DEBUG("Searching device '" << devicePtr->m_Name << "'. Searching all addresses...");
for(const auto& addressInfo : devicePtr->m_Addresses)
{
if (Logger::getInstance().isDebugEnabled(PcapLogModuleLiveDevice) && addrIter.addr != nullptr)
if (Logger::getInstance().isDebugEnabled(PcapLogModuleLiveDevice) && addressInfo.addr != nullptr)
{
std::array<char, INET6_ADDRSTRLEN> addrAsString;
internal::sockaddr2string(addrIter.addr, addrAsString.data(), addrAsString.size());
internal::sockaddr2string(addressInfo.addr, addrAsString.data(), addrAsString.size());
PCPP_LOG_DEBUG("Searching address " << addrAsString.data());
}

in6_addr* currAddr = internal::try_sockaddr2in6_addr(addrIter.addr);
in6_addr* currAddr = internal::try_sockaddr2in6_addr(addressInfo.addr);
if (currAddr == nullptr)
{
PCPP_LOG_DEBUG("Address is nullptr");
Expand All @@ -323,7 +334,7 @@ PcapLiveDevice* PcapLiveDeviceList::getPcapLiveDeviceByIp(const IPv6Address& ip6
if (*currAddr == ip6Addr)
{
PCPP_LOG_DEBUG("Found matched address!");
return devIter;
return devicePtr.get();
}
}
}
Expand Down Expand Up @@ -353,15 +364,15 @@ PcapLiveDevice* PcapLiveDeviceList::getPcapLiveDeviceByName(const std::string& n
{
PCPP_LOG_DEBUG("Searching all live devices...");
auto devIter = std::find_if(m_LiveDeviceList.begin(), m_LiveDeviceList.end(),
[&name](const PcapLiveDevice *dev) { return dev->getName() == name; });
[&name](const std::shared_ptr<PcapLiveDevice>& dev) { return dev->getName() == name; });

if (devIter == m_LiveDeviceList.end())
{
PCPP_LOG_DEBUG("Found no live device with name '" << name << "'");
return nullptr;
}

return *devIter;
return devIter->get();
}

PcapLiveDevice* PcapLiveDeviceList::getPcapLiveDeviceByIpOrName(const std::string& ipOrName) const
Expand All @@ -384,11 +395,7 @@ PcapLiveDeviceList* PcapLiveDeviceList::clone()

void PcapLiveDeviceList::reset()
{
for(auto devIter : m_LiveDeviceList)
{
delete devIter;
}

m_LiveDeviceListView.clear();
m_LiveDeviceList.clear();
m_DnsServers.clear();

Expand Down
Loading