Skip to content

Remove possibility of setFilter and clearFilter shadowing in derived devices. #2005

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

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
Open

Remove possibility of setFilter and clearFilter shadowing in derived devices. #2005

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c70e8dd
Updates the `IFilterableDevice` interface to utilize a NVI pattern.
Dimi1010 Oct 25, 2025
8f12e7c
Updated PcapNgFileReaderDevice.
Dimi1010 Oct 25, 2025
8e97480
Fix inconsistent override warning.
Dimi1010 Oct 25, 2025
0afbd9f
Merge branch 'dev' into refactor/set-filter-shadowing
Dimi1010 Nov 16, 2025
46f0374
Update `doUpdateFilter` to account for possible valid `""` filter str…
Dimi1010 Nov 19, 2025
2b78676
Fix
Dimi1010 Nov 19, 2025
e476641
Fix
Dimi1010 Nov 19, 2025
ea58bde
Typo.
Dimi1010 Nov 19, 2025
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
27 changes: 22 additions & 5 deletions Pcap++/header/Device.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,22 +53,39 @@ namespace pcpp
/// received
/// @param[in] filter The filter to be set in PcapPlusPlus' GeneralFilter format
/// @return True if filter set successfully, false otherwise
virtual bool setFilter(GeneralFilter& filter)
bool setFilter(GeneralFilter& filter)
{
std::string filterAsString;
filter.parseToString(filterAsString);
return setFilter(filterAsString);
return doUpdateFilter(&filterAsString);
}

/// Set a filter for the device. When implemented by the device, only packets that match the filter will be
/// received
/// processed.
/// @param[in] filterAsString The filter to be set in Berkeley Packet Filter (BPF) syntax
/// (http://biot.com/capstats/bpf.html)
/// @return True if filter set successfully, false otherwise
virtual bool setFilter(std::string filterAsString) = 0;
bool setFilter(std::string const& filterAsString)
{
return doUpdateFilter(&filterAsString);
}

/// Clear the filter currently set on the device
/// @return True if filter was removed successfully or if no filter was set, false otherwise
virtual bool clearFilter() = 0;
bool clearFilter()
Copy link
Owner

@seladb seladb Nov 16, 2025

Choose a reason for hiding this comment

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

The problem as presented doesn't apply to clearFilter() because it only has one overload so nothing is shadowed. Why not keep it abstract?

Using "" as an indication to clear the filter seems a bit weird to me. I think clearFilter() is more explicit

Copy link
Collaborator Author

@Dimi1010 Dimi1010 Nov 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The problem as presented doesn't apply to clearFilter() because it only has one overload so nothing is shadowed. Why not keep it abstract?

Simplification of the internal interface mostly. Having a single extension point for a certain functionality makes it harder to forget about part of it when overriding. This had happened in a couple of places already.

In the old code PcapNgFileReaderDevice and PcapNgFileWriterDevice only overrode setFilter to target m_BpfWrapper. Calling clearFilter would have had no effect on the filter, since that still used the IPcapDevice::clearFilter implementation that worked on m_PcapDescriptor.

My reasoning was that both setFilter and clearFilter can be conceptualized by the primitive action "update the filter", just with different parameters. This was also a factor when deciding to make the hook as doUpdateFilter and not for example doSetFilter.

The single extension point makes the above issue impossible, since to override the filter mechanism you have to update the entire hook. There is nothing to forget to override.

Afterwards clearFilter was made non-virtual to remove the possibility of overriding the method, thus potentially bypassing the base implementation, which is undesirable. Under the design derived classes should only modify the hook implementation.

Using "" as an indication to clear the filter seems a bit weird to me. I think clearFilter() is more explicit

For public API, I agree, clearFilter is more user friendly.

However, I primary made the the hook signature to fit the aforementioned design goals. It wasn't designed to be exposed to the public.

The choice of "" for "no filter" was due to PcapHandle and libpcap already treating it as match any packet, when passed to pcap_compile so I thought it might fit.

Copy link
Owner

@seladb seladb Nov 19, 2025

Choose a reason for hiding this comment

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

I agree that a single abstract method is simpler than two when it comes to overriding and making sure one doesn't accidentally forget to override the other, but when looking at the implementation, in which "" symbolizes clearing a filter, I think that clearFilter is better. It is an internal API, but it might be used by anyone who wants to extend our packet capture capabilities (including our future selves 🙂 ). Using doUpdateFilter("") seems a bit rigid and might limit future functionality (for example - if a "" is a valid filter in one of the packet engines).

I suggest we keep clearFilter an abstract method - it doesn't make things much more complex and at the same time provides better flexibility.

Copy link
Collaborator Author

@Dimi1010 Dimi1010 Nov 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Fair enough. I see the reasoning about potentially "" being valid filter.

Tbh, this is one of those times where C++17's std::optional would be really nice. We could have doUpdateFilter(std::optional<std::string> filter).

Maybe we can have a simple struct that mimics it until we upgrade to 17? I would prefer to keep the number of virtual methods to a minimum if we can.

string const* would techinially work too since we don't need to store the value (or we would copy it anyway), nullptr being no filter. It would be better from the no copy perspective compared to optional.

If we can't, I guess we can keep the clearFilter abstract until c++17.

Copy link
Collaborator Author

@Dimi1010 Dimi1010 Nov 19, 2025

Choose a reason for hiding this comment

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

Updated doUpdateFilter to take std::string const* filterStr as an filter parameter, with nullptr being the "no-filter" value. This should allow "" to be used in an implementation defined way, while minimising the virtual interface surface area.

46f0374

{
return doUpdateFilter(nullptr);
}

protected:
/// @brief Updates the filter on the device with a BPF string.
///
/// Only packets that match the filter should be processed by the device after this method is called.
/// A nullptr should disable any existing filter on the device.
///
/// @param filterAsString A pointer to a string representing the filter in BPF syntax
/// (http://biot.com/capstats/bpf.html).
/// @return True if the operation was successful, false otherwise.
virtual bool doUpdateFilter(std::string const* filterAsString) = 0;
};
} // namespace pcpp
17 changes: 2 additions & 15 deletions Pcap++/header/PcapDevice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,20 +164,7 @@ namespace pcpp
PCPP_DEPRECATED("Prefer GeneralFilter::matches(...) method directly.")
static bool matchPacketWithFilter(GeneralFilter& filter, RawPacket* rawPacket);

// implement abstract methods

using IFilterableDevice::setFilter;

/// Set a filter for the device. When implemented by the device, only packets that match the filter will be
/// received. Please note that when the device is closed the filter is reset so when reopening the device you
/// need to call this method again in order to reactivate the filter
/// @param[in] filterAsString The filter to be set in Berkeley Packet Filter (BPF) syntax
/// (http://biot.com/capstats/bpf.html)
/// @return True if filter set successfully, false otherwise
bool setFilter(std::string filterAsString) override;

/// Clear the filter currently set on device
/// @return True if filter was removed successfully or if no filter was set, false otherwise
bool clearFilter() override;
protected:
bool doUpdateFilter(std::string const* filterAsString) override;
};
} // namespace pcpp
22 changes: 8 additions & 14 deletions Pcap++/header/PcapFileDevice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,21 +363,18 @@ namespace pcpp
/// @param[out] rawPacket A reference for an empty RawPacket where the packet will be written
/// @return True if a packet was read successfully. False will be returned if the file isn't opened (also, an
/// error log will be printed) or if reached end-of-file
bool getNextPacket(RawPacket& rawPacket);
bool getNextPacket(RawPacket& rawPacket) override;

/// Open the file name which path was specified in the constructor in a read-only mode
/// @return True if file was opened successfully or if file is already opened. False if opening the file failed
/// for some reason (for example: file path does not exist)
bool open();

/// Set a filter for PcapNG reader device. Only packets that match the filter will be received
/// @param[in] filterAsString The filter to be set in Berkeley Packet Filter (BPF) syntax
/// (http://biot.com/capstats/bpf.html)
/// @return True if filter set successfully, false otherwise
bool setFilter(std::string filterAsString);
bool open() override;

/// Close the pacp-ng file
void close();
void close() override;

protected:
bool doUpdateFilter(std::string const* filter) override;
};

/// @class PcapNgFileWriterDevice
Expand Down Expand Up @@ -476,11 +473,8 @@ namespace pcpp
/// Flush and close the pcap-ng file
void close() override;

/// Set a filter for PcapNG writer device. Only packets that match the filter will be persisted
/// @param[in] filterAsString The filter to be set in Berkeley Packet Filter (BPF) syntax
/// (http://biot.com/capstats/bpf.html)
/// @return True if filter set successfully, false otherwise
bool setFilter(std::string filterAsString) override;
protected:
bool doUpdateFilter(std::string const* filterAsString) override;

private:
/// @struct PcapNgMetadata
Expand Down
13 changes: 5 additions & 8 deletions Pcap++/header/PfRingDevice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,15 +315,12 @@ namespace pcpp
return m_DeviceOpened;
}

using IFilterableDevice::setFilter;
protected:
bool doUpdateFilter(std::string const* filterAsString) override;

/// Sets a BPF filter to the device
/// @param[in] filterAsString The BPF filter in string format
bool setFilter(std::string filterAsString);

/// Remove a filter if currently set
/// @return True if filter was removed successfully or if no filter was set, false otherwise
bool clearFilter();
private:
bool removeFilterForAllChannels();
bool setFilterForAllChannels(std::string const& filterAsString);
};

} // namespace pcpp
Expand Down
16 changes: 9 additions & 7 deletions Pcap++/src/PcapDevice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ namespace pcpp
return stats;
}

bool IPcapDevice::setFilter(std::string filterAsString)
bool IPcapDevice::doUpdateFilter(std::string const* filterAsString)
{
PCPP_LOG_DEBUG("Filter to be set: '" << filterAsString << "'");
if (!isOpened())
Expand All @@ -142,12 +142,14 @@ namespace pcpp
return false;
}

return m_PcapDescriptor.setFilter(filterAsString);
}

bool IPcapDevice::clearFilter()
{
return m_PcapDescriptor.clearFilter();
if (filterAsString == nullptr || filterAsString->empty())
{
return m_PcapDescriptor.clearFilter();
}
else
{
return m_PcapDescriptor.setFilter(*filterAsString);
}
}

bool IPcapDevice::matchPacketWithFilter(GeneralFilter& filter, RawPacket* rawPacket)
Expand Down
18 changes: 14 additions & 4 deletions Pcap++/src/PcapFileDevice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,9 +624,14 @@ namespace pcpp
return getNextPacket(rawPacket, temp);
}

bool PcapNgFileReaderDevice::setFilter(std::string filterAsString)
bool PcapNgFileReaderDevice::doUpdateFilter(std::string const* filterAsString)
{
return m_BpfWrapper.setFilter(filterAsString);
if (filterAsString == nullptr)
{
return m_BpfWrapper.setFilter("");
}

return m_BpfWrapper.setFilter(*filterAsString);
}

void PcapNgFileReaderDevice::close()
Expand Down Expand Up @@ -876,9 +881,14 @@ namespace pcpp
PCPP_LOG_DEBUG("File writer closed for file '" << m_FileName << "'");
}

bool PcapNgFileWriterDevice::setFilter(std::string filterAsString)
bool PcapNgFileWriterDevice::doUpdateFilter(std::string const* filterAsString)
{
return m_BpfWrapper.setFilter(filterAsString);
if (filterAsString == nullptr)
{
return m_BpfWrapper.setFilter("");
}

return m_BpfWrapper.setFilter(*filterAsString);
}

// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Expand Down
16 changes: 14 additions & 2 deletions Pcap++/src/PfRingDevice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,14 +366,26 @@ namespace pcpp
return SystemCores::IdToSystemCore[sched_getcpu()];
}

bool PfRingDevice::setFilter(std::string filterAsString)
bool PfRingDevice::doUpdateFilter(std::string const* filterAsString)
{
if (!m_DeviceOpened)
{
PCPP_LOG_ERROR("Device not opened");
return false;
}

if (filterAsString == nullptr || filterAsString->empty())
{
return removeFilterForAllChannels();
}
else
{
return setFilterForAllChannels(*filterAsString);
}
}

bool PfRingDevice::setFilterForAllChannels(std::string const& filterAsString)
{
for (pfring* rxChannel : m_PfRingDescriptors)
{
int res = pfring_set_bpf_filter(rxChannel, (char*)filterAsString.c_str());
Expand All @@ -394,7 +406,7 @@ namespace pcpp
return true;
}

bool PfRingDevice::clearFilter()
bool PfRingDevice::removeFilterForAllChannels()
{
if (!m_IsFilterCurrentlySet)
return true;
Expand Down
Loading