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

Refactor of PcapRemoteDeviceList's factory functions. #1476

Merged
merged 10 commits into from
Jul 17, 2024
Merged

Refactor of PcapRemoteDeviceList's factory functions. #1476

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
af785be
Refactor PcapRemoteDeviceList with smart pointers
Dimi1010 Jun 30, 2024
d610fbe
Refactor iterator type aliases in PcapRemoteDeviceList
Dimi1010 Jun 30, 2024
6f5cc0d
Enhance PcapRemoteDeviceList construction
Dimi1010 Jun 30, 2024
1a257b7
Removed unused setter methods.
Dimi1010 Jun 30, 2024
6916da3
Refactor PcapRemoteDeviceList creation
Dimi1010 Jun 30, 2024
edfa5de
Clarified variable name.
Dimi1010 Jul 3, 2024
4f66704
Renamed sRemoteAuth to remoteAuthCopy.
Dimi1010 Jul 3, 2024
2d637c4
Merge remote-tracking branch 'upstream/dev' into refactor/remote-list…
Dimi1010 Jul 3, 2024
0f8f3b0
Merge remote-tracking branch 'upstream/dev' into refactor/remote-list…
Dimi1010 Jul 7, 2024
cf3d1c2
Updated variable name to conform with pointer naming conventions.
Dimi1010 Jul 7, 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
59 changes: 51 additions & 8 deletions Pcap++/header/PcapRemoteDeviceList.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <memory>
#include "IpAddress.h"
#include "PcapRemoteDevice.h"
#include "DeprecationUtils.h"

/// @file

Expand Down Expand Up @@ -33,23 +34,21 @@ namespace pcpp
uint16_t m_RemoteMachinePort;
std::shared_ptr<PcapRemoteAuthentication> m_RemoteAuthentication;

// private c'tor. User should create the list via static methods PcapRemoteDeviceList::getRemoteDeviceList()
PcapRemoteDeviceList() : m_RemoteMachinePort(0), m_RemoteAuthentication(nullptr) {}

void setRemoteMachineIpAddress(const IPAddress& ipAddress);
void setRemoteMachinePort(uint16_t port);
void setRemoteAuthentication(const PcapRemoteAuthentication* remoteAuth);
// private c'tor. User should create the list via static methods PcapRemoteDeviceList::createRemoteDeviceList()
PcapRemoteDeviceList(const IPAddress& ipAddress, uint16_t port,
std::shared_ptr<PcapRemoteAuthentication> remoteAuth,
std::vector<PcapRemoteDevice*> deviceList);

public:
/**
* Iterator object that can be used for iterating all PcapRemoteDevice in list
*/
typedef typename std::vector<PcapRemoteDevice*>::iterator RemoteDeviceListIterator;
using RemoteDeviceListIterator = typename std::vector<PcapRemoteDevice*>::iterator;

/**
* Const iterator object that can be used for iterating all PcapRemoteDevice in a constant list
*/
typedef typename std::vector<PcapRemoteDevice*>::const_iterator ConstRemoteDeviceListIterator;
using ConstRemoteDeviceListIterator = typename std::vector<PcapRemoteDevice*>::const_iterator;

PcapRemoteDeviceList(const PcapRemoteDeviceList&) = delete;
PcapRemoteDeviceList(PcapRemoteDeviceList&&) noexcept = delete;
Expand All @@ -70,9 +69,30 @@ namespace pcpp
* - IP address provided is NULL or not valid
* - WinPcap/Npcap encountered an error in creating the remote connection string
* - WinPcap/Npcap encountered an error connecting to the rpcapd daemon on the remote machine or retrieving devices on the remote machine
* @deprecated This factory function has been deprecated in favor of 'createRemoteDeviceList' factory for better memory safety.
*/
PCPP_DEPRECATED("Please use 'createRemoteDeviceList' factory method instead.")
static PcapRemoteDeviceList* getRemoteDeviceList(const IPAddress& ipAddress, uint16_t port);

/**
* A static method for creating a PcapRemoteDeviceList instance for a specific remote machine.
* This methods creates the instance and populates it with PcapRemoteDevice instances.
* Each PcapRemoteDevice instance corresponds to a network interface on the remote machine.
*
* This method overload is for remote daemons which don't require authentication for accessing them.
* For daemons which do require authentication use the other method overload.
*
* @param[in] ipAddress The IP address of the remote machine through which clients can connect to the rpcapd
* daemon
* @param[in] port The port of the remote machine through which clients can connect to the rpcapd daemon
* @return A smart pointer to the newly created PcapRemoteDeviceList, or nullptr if (an appropriate error will be printed
* to log in each case):
* - WinPcap/Npcap encountered an error in creating the remote connection string
* - WinPcap/Npcap encountered an error connecting to the rpcapd daemon on the remote machine or retrieving
* devices on the remote machine
*/
static std::unique_ptr<PcapRemoteDeviceList> createRemoteDeviceList(const IPAddress& ipAddress, uint16_t port);

/**
* An overload of the previous getRemoteDeviceList() method but with authentication support. This method is suitable for connecting to
* remote daemons which require authentication for accessing them
Expand All @@ -84,9 +104,32 @@ namespace pcpp
* - IP address provided is NULL or not valid
* - WinPcap/Npcap encountered an error in creating the remote connection string
* - WinPcap/Npcap encountered an error connecting to the rpcapd daemon on the remote machine or retrieving devices on the remote machine
* @deprecated This factory function has been deprecated in favor of 'createRemoteDeviceList' factory for better memory safety.
*/
PCPP_DEPRECATED("Please use 'createRemoteDeviceList' factory method instead.")
static PcapRemoteDeviceList* getRemoteDeviceList(const IPAddress& ipAddress, uint16_t port, PcapRemoteAuthentication* remoteAuth);

/**
* A static method for creating a PcapRemoteDeviceList instance for a specific remote machine.
* This methods creates the instance and populates it with PcapRemoteDevice instances.
* Each PcapRemoteDevice instance corresponds to a network interface on the remote machine.
*
* This method overload is for remote daemons which require authentication for accessing them.
* If no authentication is required, use the other method overload.
*
* @param[in] ipAddress The IP address of the remote machine through which clients can connect to the rpcapd
* daemon
* @param[in] port The port of the remote machine through which clients can connect to the rpcapd daemon
* @param[in] remoteAuth A pointer to the authentication object which contains the username and password for
* connecting to the remote daemon
* @return A smart pointer to the newly created PcapRemoteDeviceList, or nullptr if (an appropriate error will be printed
* to log in each case):
* - WinPcap/Npcap encountered an error in creating the remote connection string
* - WinPcap/Npcap encountered an error connecting to the rpcapd daemon on the remote machine or retrieving
* devices on the remote machine
*/
static std::unique_ptr<PcapRemoteDeviceList> createRemoteDeviceList(const IPAddress& ipAddress, uint16_t port, PcapRemoteAuthentication const* remoteAuth);

/**
* @return The IP address of the remote machine
*/
Expand Down
76 changes: 43 additions & 33 deletions Pcap++/src/PcapRemoteDeviceList.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,19 +48,40 @@ namespace pcpp
}
}

PcapRemoteDeviceList::PcapRemoteDeviceList(const IPAddress& ipAddress, uint16_t port, std::shared_ptr<PcapRemoteAuthentication> remoteAuth, std::vector<PcapRemoteDevice*> deviceList)
: m_RemoteDeviceList(std::move(deviceList))
, m_RemoteMachineIpAddress(ipAddress)
, m_RemoteMachinePort(port)
, m_RemoteAuthentication(std::move(remoteAuth))
{}

PcapRemoteDeviceList* PcapRemoteDeviceList::getRemoteDeviceList(const IPAddress& ipAddress, uint16_t port)
{
return PcapRemoteDeviceList::getRemoteDeviceList(ipAddress, port, NULL);
auto result = PcapRemoteDeviceList::createRemoteDeviceList(ipAddress, port);
return result.release();
}

std::unique_ptr<PcapRemoteDeviceList> PcapRemoteDeviceList::createRemoteDeviceList(const IPAddress& ipAddress, uint16_t port)
{
return PcapRemoteDeviceList::createRemoteDeviceList(ipAddress, port, nullptr);
}

PcapRemoteDeviceList* PcapRemoteDeviceList::getRemoteDeviceList(const IPAddress& ipAddress, uint16_t port, PcapRemoteAuthentication* remoteAuth)
{
auto result = PcapRemoteDeviceList::createRemoteDeviceList(ipAddress, port, remoteAuth);
return result.release();
}

std::unique_ptr<PcapRemoteDeviceList> PcapRemoteDeviceList::createRemoteDeviceList(const IPAddress& ipAddress, uint16_t port, PcapRemoteAuthentication const* remoteAuth)
{
std::shared_ptr<PcapRemoteAuthentication> sRemoteAuth;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like the naming sRemoteAuth, it's not the convention in this project.

Copy link
Collaborator

Choose a reason for hiding this comment

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

however, we don't have many std::shared_ptr currently. So if other people agree with this, this can become the official coding style.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Originally, I had a private overload where remoteAuth was a shared ptr to avoid that, but scrapped it as it was not strictly nessesary.

Perhaps that would be a better solution?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tbh, I just named it that way coz i needed the variable to not conflict. If there are suggestions on a potential name, i'm happy to hear them.

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 it to remoteAuthCopy to be more explicit that its a copy in 4f66704. Not sure if it should be remoteAuthCopy or pRemoteAuthCopy tho.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am fine with either, but seems pXXX is more common in the codebase?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated cf3d1c2

pcap_rmtauth* pRmAuth = nullptr;
pcap_rmtauth rmAuth;
if (remoteAuth != nullptr)
{
PCPP_LOG_DEBUG("Authentication requested. Username: " << remoteAuth->userName << ", Password: " << remoteAuth->password);
rmAuth = remoteAuth->getPcapRmAuth();
sRemoteAuth = std::make_shared<PcapRemoteAuthentication>(*remoteAuth);
rmAuth = sRemoteAuth->getPcapRmAuth();
pRmAuth = &rmAuth;
}

Expand All @@ -75,20 +96,29 @@ PcapRemoteDeviceList* PcapRemoteDeviceList::getRemoteDeviceList(const IPAddress&
return nullptr;
}

PcapRemoteDeviceList* resultList = new PcapRemoteDeviceList();
resultList->setRemoteMachineIpAddress(ipAddress);
resultList->setRemoteMachinePort(port);
resultList->setRemoteAuthentication(remoteAuth);


for (pcap_if_t* currInterface = interfaceList.get(); currInterface != nullptr; currInterface = currInterface->next)
std::vector<PcapRemoteDevice*> devices;
try
{
for (pcap_if_t* currInterface = interfaceList.get(); currInterface != nullptr; currInterface = currInterface->next)
{
auto pNewRemoteDevice = std::unique_ptr<PcapRemoteDevice>(new PcapRemoteDevice(currInterface, sRemoteAuth, ipAddress, port));
// Release is called after pushback to prevent memory leaks if vector reallocation fails.
// cppcheck-suppress danglingLifetime
devices.push_back(pNewRemoteDevice.get());
pNewRemoteDevice.release();
}
}
catch (const std::exception& e)
{
PcapRemoteDevice* pNewRemoteDevice = new PcapRemoteDevice(currInterface, resultList->m_RemoteAuthentication,
resultList->getRemoteMachineIpAddress(), resultList->getRemoteMachinePort());
resultList->m_RemoteDeviceList.push_back(pNewRemoteDevice);
for (auto dev : devices)
tigercosmos marked this conversation as resolved.
Show resolved Hide resolved
{
delete dev;
}
PCPP_LOG_ERROR("Error creating remote devices: " << e.what());
return nullptr;
}

return resultList;
return std::unique_ptr<PcapRemoteDeviceList>(new PcapRemoteDeviceList(ipAddress, port, sRemoteAuth, devices));
}

PcapRemoteDevice* PcapRemoteDeviceList::getRemoteDeviceByIP(const std::string& ipAddrAsString) const
Expand Down Expand Up @@ -190,26 +220,6 @@ PcapRemoteDevice* PcapRemoteDeviceList::getRemoteDeviceByIP(const IPv6Address& i

}

void PcapRemoteDeviceList::setRemoteMachineIpAddress(const IPAddress& ipAddress)
{
m_RemoteMachineIpAddress = ipAddress;
}

void PcapRemoteDeviceList::setRemoteMachinePort(uint16_t port)
{
m_RemoteMachinePort = port;
}

void PcapRemoteDeviceList::setRemoteAuthentication(const PcapRemoteAuthentication* remoteAuth)
{
if (remoteAuth != nullptr)
m_RemoteAuthentication = std::shared_ptr<PcapRemoteAuthentication>(new PcapRemoteAuthentication(*remoteAuth));
else
{
m_RemoteAuthentication = nullptr;
}
}

PcapRemoteDeviceList::~PcapRemoteDeviceList()
{
while (m_RemoteDeviceList.size() > 0)
Expand Down
Loading