Use file content heuristics to decide file reader. #1962
Conversation
…sed on the magic number.
…ics detection method.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #1962 +/- ##
==========================================
+ Coverage 83.41% 83.49% +0.08%
==========================================
Files 311 312 +1
Lines 55019 54807 -212
Branches 11816 11839 +23
==========================================
- Hits 45892 45762 -130
+ Misses 7852 7802 -50
+ Partials 1275 1243 -32
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Dimi1010
added
the
API deprecation
Tests/Pcap++Test/Tests/FileTests.cpp
Outdated
| PTF_ASSERT_NOT_NULL(dynamic_cast<pcpp::PcapNgFileReaderDevice*>(genericReader)); | ||
| PTF_ASSERT_TRUE(genericReader->open()); | ||
| // ------- IFileReaderDevice::createReader() Factory | ||
| // TODO: Move to a separate unit test. |
There was a problem hiding this comment.
We should add the following to get more coverage:
- Open a snoop file
- Open a file that is not any of the options
- Open pcap files with different magic numbers
- Assuming we add a version check for snoop and pcap file: create temp files with bogus data that has the magic number but wrong versions
There was a problem hiding this comment.
3d713ab adds the following tests:
- Pcap, PcapNG, Zst file with correct content + extension
- Pcap, PcanNG file with correct content + wrong extension
- Bogus content file with correct extension (pcap, pcapng, zst)
- Bogus content file with wrong extension (txt)
Haven't found a snoop file to add. Do we have any?
Open pcap files with different magic numbers
Do you mean Pcap content that has just its magic number changed? Because IMO it is reasonable to consider that invalid format and fail as regular bogus data.
Assuming we add a version check for snoop and pcap file: create temp files with bogus data that has the magic number but wrong versions
Pending on #1962 (comment) .
Move it out if it needs to be reused somewhere.
Libpcap supports reading this format since 0.9.1. The heuristics detection will identify such magic number as pcap and leave final support decision to the pcap backend infrastructure.
|
@Dimi1010 some CI tests fail... |
…on 1 line and doxygen errors when its in 2 lines.
Pcap++/header/PcapFileDevice.h
Outdated
| enum class CaptureFileFormat | ||
| { | ||
| Unknown, | ||
| Pcap, // regular pcap with microsecond precision | ||
| PcapNano, // regular pcap with nanosecond precision | ||
| PcapNG, // uncompressed pcapng | ||
| PcapNGZstd, // zstd compressed pcapng | ||
| Snoop, // solaris snoop | ||
| }; | ||
|
|
||
| /// @brief Heuristic file format detector that scans the magic number of the file format header. | ||
| class CaptureFileFormatDetector |
There was a problem hiding this comment.
If I'm not mistaken, this used to be in the .cpp file, right? Is the reason we moved it to the .h file is to make it easier to test?
If yes, I think we can test it using createReader() - create a temporary fake file with the data we want to test, and delete it when the test is done
There was a problem hiding this comment.
I tried that suggestion initially, but it would have been an extremely fragile unit test. The "pass" conditions would have been checked indirectly.
Also, createReader has multiple return paths for Nano / Zst file formats, which would have caused complications since the format test would have needed to care about the environment it runs at, which it doesn't have to as a standalone.
Any additional changes to createReader could also break the test, which they really shouldn't. For example, I am thinking of maybe adding additional logic for Zst archive to check if the compressed data is actually a pcapng, and not a random file. This would be a nightmare to make compatible with the "spoofed files" test due to assumptions on the test that createReader doesn't do anything more complicated than check the initial magic number.
So, in the end, you end up with a more compilcated unit test to read through that:
- depends on the environment it runs on.
- can be broken not just by changes to the format detector but also changes to the
createReaderfactory, too. - induces requirements on
createReaderas it uses its behavior to testdetectFormat.
There was a problem hiding this comment.
I understand it's better to test CaptureFileFormatDetector as a standalone class, but it requires exposing it in the .h file which is not great (even though it's in the internal namespace). Testing createReader is a bit more fragile, but I don't think the difference is that big. Of course, if we add logic to detect more file types or update the existing detection logic some tests might break, but we easily fix them as needed.
I usually try to avoid the internal namespace where possible because it's still in the .h file and is exposed to users, and we'd like to keep our API as clean as possible
There was a problem hiding this comment.
Testing createReader is a bit more fragile, but I don't think the difference is that big. Of course, if we add logic to detect more file types or update the existing detection logic some tests might break, but we easily fix them as needed.
It is a big difference and it's not always an easy fix. I plan to add the aforementioned Zst checks in another PR after this one, and that would make zst spoofing in createReader impossible, due to zst format automatically being checked for PcapNg or Unknown contents. Therefor you can't rely on the return of createReader to find out what the return of detectFormat was, because nullptr can be returned from several paths from detectFormat return value (Unknown, Nano + unsupported, Zst + unsupported). We have already had issues with tests being silently broken (#1977 comes to mind), so I would prefer to avoid fragile tests if we can.
I usually try to avoid the internal namespace where possible because it's still in the .h file and is exposed to users, and we'd like to keep our API as clean as possible
Fair, it is exposed, but the that is the entire reason of having the internal namespace. It is a common convention that external users shouldn't really touch it. If you want to keep the primary public header files clean there are a couple options:
- I have seen many libraries have a subfolder
internal/detailin their public include folder, where they keep all their internal code headers that need to be exposed. That keeps the "internal" code separate from the "public" code, if users want to read through the headers. This is a common convention used in Boost libraries. "public" headers that depend on internal headers include them from theinternalsubfolder. - In the current case, we have another option. Since the
CaptureFileFormatDetectoris only needed in thecpppart and not in the header part, we can extract it to a fully internal header, kept with the source files. This would prevent it from being exposed in the public API, but the Test project can be manually set to search for headers from "Pcap++/src" too, to allow it to link in the tests.
There was a problem hiding this comment.
It is a big difference and it's not always an easy fix. I plan to add the aforementioned Zst checks in another PR after this one, and that would make zst spoofing in
createReaderimpossible, due to zst format automatically being checked for PcapNg or Unknown contents. Therefor you can't rely on the return ofcreateReaderto find out what the return ofdetectFormatwas, becausenullptrcan be returned from several paths fromdetectFormatreturn value (Unknown, Nano + unsupported, Zst + unsupported). We have already had issues with tests being silently broken (#1977 comes to mind), so I would prefer to avoid fragile tests if we can.
I'm not sure I understand... if we create fake files we know which type to expect, so all the test needs to do is verify the created file device is of the expected type 🤔
- In the current case, we have another option. Since the
CaptureFileFormatDetectoris only needed in thecpppart and not in the header part, we can extract it to a fully internal header, kept with the source files. This would prevent it from being exposed in the public API, but the Test project can be manually set to search for headers from "Pcap++/src" too, to allow it to link in the tests.
I guess we can do that, but I still don't understand why we can't test it with createReader or tryCreateReader
There was a problem hiding this comment.
Tbh, I think that unit tests should mostly validate public API for an object. Not
private, as that is compilated.
For me what guides me is the Single Responsibility Principle:
* `createReader` factory has the sole responsibility of creating the correct reader for a given file format. * `CaptureFileFormatDetector::detectFormat` has the sole responsibility of determining the file format of a given byte stream.
These 2 statements sometimes contradict, and this is a good example: on one hand createReader is the public API which you claim we should test, while CaptureFileFormatDetector is considered private and shouldn't be tested, on the other hand, the Single Responsibility Principle dictates testing this "private" code. However, since C++ doesn't allow accessing private/protected methods, I usually examine it case-by-case. In this case I think it's ok to only test createReader because it covers CaptureFileFormatDetector well
Moving the format detector to a different file for tests is a relatively minor change, IMO. It is the same as moving the
ObjectPoolto a different file fromLoggereven though it is only used there, no? The only difference is that I initially made it local to thePcapFileDevice.cppas it didn't have test then.Having it as a non-public header does make it a bit harder to include in the tests project, but it is a minor workaround, as it would only be included once for its tests.
ObjectPool is different because it's not specific to the Logger and can be reused elsewhere. CaptureFileFormatDetector is very specific to this file, so extracting it to its own .h and .cpp files just for the sake of testing seems redundant to me
There was a problem hiding this comment.
Tbh, I think that unit tests should mostly validate public API for an object. Not
private, as that is compilated.For me what guides me is the Single Responsibility Principle:
* `createReader` factory has the sole responsibility of creating the correct reader for a given file format. * `CaptureFileFormatDetector::detectFormat` has the sole responsibility of determining the file format of a given byte stream.These 2 statements sometimes contradict, and this is a good example: on one hand
createReaderis the public API which you claim we should test, whileCaptureFileFormatDetectoris considered private and shouldn't be tested, on the other hand, the Single Responsibility Principle dictates testing this "private" code. However, since C++ doesn't allow accessing private/protected methods, I usually examine it case-by-case. In this case I think it's ok to only testcreateReaderbecause it coversCaptureFileFormatDetectorwell
Sometimes they can, yes, but key word here is "for an object". What is public depends on the context you are looking at:
- If you are looking at the library as a whole, what is considered public is what is in the public headers. The rest are internal headers.
- If you are looking at a specific class, what is considered public is the class's public methods.
Generally, unit tests should validate code behaviour in the most isolated case that can be achieved (that isn't a trivial case) to keep them simple and so you can easily locate the error. That means that cases should be isolated enough that, in the case of failure to show the smallest reasonable region of where the error is, not just that the error exists (example below). As such, the context most looked at should be option 2 as that generally keeps the individual tests more isolated and on the smaller size. The format detector's logic is complex enough that it justifies for as a separate unit.
[...] while
CaptureFileFormatDetectoris considered private and shouldn't be tested, on the other hand, the Single Responsibility Principle dictates testing this "private" code.
Under option 2, CaptureFileFormatDetector is not considered "private" but a separate object that must be validated independently as it is not trivial.
However, since C++ doesn't allow accessing private/protected methods, I usually examine it case-by-case.
In this case, we have no such issues, as the format detector is a separate reusable object, and not a private method of IFileReaderDevice. The only issue is that of linkage, as the tests should be able to link to the format detector. C++ allows that naturally by having it as external linkage, but you don't want it exposed to the public API (as it is rightfully not needed for the users), therefor separate header that is considered "internal", not "private".
In the current case "I think that unit tests should mostly validate public API for an object" means that the format detector should only have a unit test for detectFormat and not for all the implementations of private methods detectPcap, isPcapNG, etc.
Here is an example:
// This should be tested in a standalone case with respect to Bar unit tests.
// Assume this is not a trivial computation.
int bar(int a) {
if (a < 0)
return 0;
return a + 42;
}
class Foo
{
public:
// This should be tested in a standalone case with respect to Foo unit tests.
int doFoo(int a) {
if (validateA(a))
{
return doFooImpl(a);
}
return 0;
}
private:
// This should NOT be tested in a standalone case, as it is considered "private" implementation.
int doFooImpl(int a) {
auto barVal = bar(a);
if (barVal > 50)
{
return barVal + 5;
}
return barVal + 10;
}
// This should NOT be tested in a standalone case, as it is considered "private" implementation
bool validateA(int a) {
return a >= 5;
}
}In the above case, you have 2 logical independent reusable units: Bar and Foo.
Foo's unit tests should concert themselves only with validating the input / output behaviour of doFoo and other public methods of Foo. It should not concern itself with validating bar()'s behaviour, because:
- Testing
bar()is testing the precise internal implementation ofdoFoo(), instead of just validating externally observable behaviour. - It can't do it cleanly:
doFoo(...)'s valid range is [5, IntMax], whilebar(...)'s valid range is [0, IntMax]. How do you test the other values?
For the aforementioned statement:
That means that cases should be isolated enough that, in the case of failure to show the smallest reasonable region of where the error is, not just that the error exists (example below).
If Foo tests fail, but Bar tests pass, that means that the error is in Foo, if only Foo tests existed, then we would need to also go look through bar(...) since we wouldn't know that it runs fine.
In summary:
Foo's unit tests should treat everything underFoo::doFoo(int a)as a black box, and only validate the externally observable behaviour.- In its turn,
Fooshould treatbar(...)as a black box that does what it says on the tin can. - Bar's unit tests should validate the full input / output behaviour of
bar(int a)(so it actually does what it says it does) and treatbar's implementation of how it achieves that as a black box.
Now replace Foo::doFoo with createReader and bar with detectFormat.
In this case I think it's ok to only test
createReaderbecause it coversCaptureFileFormatDetectorwell
It does not cover it well, considering we would need to resort to workarounds such as creating nominally invald fake files that should realistically be discarded by the factory validation, as they can't produce a valid reader device.
The format detector might work with them, because of its current implementation only scans the first 4 to 6 bytes, but that does not mean that the reader factory should.
Realistically, after the format is determined and a device is created, the factory should attempt to open the file to read it to validate it can before returning the reader (and optionally closing it). It would not be able to do that, if it has to accept those fake files for the purposes of testing something that should be considered a black box that "just works" from its perspective. Allowing those "fake" files locks us out of any other potential validation we might be able to do on the files, when there are cleaner solutions that don't lock us out of it. That or we now need to modify the fake files again, which is also more work that does not happen with separate unit tests.
ObjectPoolis different because it's not specific to theLoggerand can be reused elsewhere.CaptureFileFormatDetectoris very specific to this file, so extracting it to its own.hand.cppfiles just for the sake of testing seems redundant to me
I agree that it was specifically made for the factory's needs, but not that it is specific to it.
The format detector is perfectly viable to be viewed as a black box from the factory's perspective. File contents go in, format enum value goes out. That same black box can be reused elsewhere if needed, making it independent from the factory's implementation.
There was a problem hiding this comment.
Generally, unit tests should validate code behaviour in the most isolated case that can be achieved (that isn't a trivial case) to keep them simple and so you can easily locate the error. That means that cases should be isolated enough that, in the case of failure to show the smallest reasonable region of where the error is, not just that the error exists (example below). As such, the context most looked at should be option 2 as that generally keeps the individual tests more isolated and on the smaller size. The format detector's logic is complex enough that it justifies for as a separate unit.
I don't disagree on the concept, however the tradeoff I'm trying to make is between:
- Refactor code just for the sake of writing tests
- Be able to write tests
- Write tests that are isolated enough, as you mentioned
If (2) or (3) are impossible, then we may consider (1), but if they are feasible, even if not perfect, we should try to avoid (1). For example: if I'd re-write the DPDK device, I'd probably use dependency injection so we can mock the DPDK API and test at least some of our business logic.
If the code-under-test is completely isolated from the business logic (for example: ObjectPool), then (1) is not true, and it makes sense to move this code to separate header/source files.
Under option 2,
CaptureFileFormatDetectoris not considered "private" but a separate object that must be validated independently as it is not trivial.
Yes, you're right, I'm sorry for the inaccurate wording. What I meant is that CaptureFileFormatDetector is not part of the public API, but rather an internal class
In this case, we have no such issues, as the format detector is a separate reusable object, and not a private method of
IFileReaderDevice. The only issue is that of linkage, as the tests should be able to link to the format detector. C++ allows that naturally by having it as external linkage, but you don't want it exposed to the public API (as it is rightfully not needed for the users), therefor separate header that is considered "internal", not "private".
Again - the main thing that guides me is: do we need this refactoring just for the sake of testing? The answer in this case is "yes".
Regarding the example you gave: if bar is completely isolated from Foo and may be reusable in other places (like ObjectPool), then sure - we can move bar to a separate header/source file. But if bar is an internal implementation of Foo, and we have a good enough way to test doFooImpl / bar using the public method doFoo then I'd go with this approach
There was a problem hiding this comment.
If (2) or (3) are impossible, then we may consider (1), but if they are feasible, even if not perfect, we should try to avoid (1). For example: if I'd re-write the DPDK device, I'd probably use dependency injection so we can mock the DPDK API and test at least some of our business logic.
Sure, I don't disagree that refactoring just for writing tests for existing features is not desired and I am not suggesting a rewrite of existing code or tests.
I think our main disagreement is what is considered an existing feature? I consider the format detector a new feature, independent of the createReader factory's business logic. It's use might be internal to the library but it is still separate from other existing features. For disclosure, I consider a feature "existing" only after it has been merged to a main branch (e.g. dev or master).
I view the PR as the following sequence of changes:
- Add new feature to allow detection of files based on file content (format detector).
- Add tests for new feature.
- Refactor
createReaderto utilize the new feature to improve functionality.
This is why I labeled the PR as enhancement, not refactor.
Nowhere in those steps do I think we are restructuring existing code for the sole reason of testing existing features.
If the code-under-test is completely isolated from the business logic (for example: ObjectPool), then (1) is not true, and it makes sense to move this code to separate header/source files.
How do you define complete isolation? To not depend on anything? To not be related to anything? To be reusable?
IMO, the format detector is isolated enough to be justified:
- It does not depend on any business logic. Business logic depends on it. Whether we create
PcapDeviceorPcapNGDeviceis completely independent from the precise way of how we determine the format, only that we do. - Unless the piece of code is unused, I don't think you can get a completely isolated piece of code, if it is supposed to not be related to anything.
- The format detector is reusable. It currently might not be needed to be reused anywhere else, but that does not stop it from being reused. I can copy + paste or include the implementation into a program that loops through all files and counts how many Pcap files are in a folder with zero changes to the detector's implementation.
What I meant is that CaptureFileFormatDetector is not part of the public API, but rather an internal class
It is an internal class. The way I think about it, the collection of all internal classes are a micro library that the public API's implementation depends on and should be validated independently if possible with minimal complications. Having a separate header + source for a class is not a large complication.
Again - the main thing that guides me is: do we need this refactoring just for the sake of testing? The answer in this case is "yes".
As I said in my first paragraph, the way I see it, this is not refactoring of existing code just for the sake of testing, but deciding how a new independent feature will be merged, alongside all its relevant tests to validate its integrity.
Regarding the example you gave: if bar is completely isolated from Foo and may be reusable in other places (like ObjectPool), then sure - we can move bar to a separate header/source file. But if bar is an internal implementation of Foo, and we have a good enough way to test doFooImpl / bar using the public method doFoo then I'd go with this approach
Again, what do you define as "completely isolated"? For "may be reusable in other places", I think I answered that previously with the fact that I can 100% reuse the format detector wherever I need to differentiate between Pcap, PcapNG, Snoop files with minimal to no changes.
The only thing I might change is that PcapNGZst should return ZstArchive and the Zst to PcapNG folding should be done in the business logic of createReader, as it is not related to the detection of the format of the active file.
I think having that change will eliminate any mention of the necessary business logic in relation to createReader from the format detector internals, making it a fully independent module, similar to ObjectPool.
Updated: 3643fac
There was a problem hiding this comment.
This is why I labeled the PR as enhancement, not refactor.
Nowhere in those steps do I think we are restructuring existing code for the sole reason of testing existing features.
If it wasn't for the sake of testing - would you include CaptureFileFormatDetector.h as a separate file or have its implementation within PcapFileDevice.cpp? I think I know the answer 🙂 because it was initially inside PcapFileDevice.cpp and we only extracted it to a separate file for the sake of the tests...
How do you define complete isolation? To not depend on anything? To not be related to anything? To be reusable?
I think that in most cases - a piece of infrastructure code that is not related to any specific business logic and is reusable. Most of what we have in Common++ is like that. But CaptureFileFormatDetector is very specific and will only be used in the pcap reader file
It is an internal class. The way I think about it, the collection of all internal classes are a micro library that the public API's implementation depends on and should be validated independently if possible with minimal complications. Having a separate header + source for a class is not a large complication.
I agree it's not a large complication, but we almost never do it in PcapPlusPlus, and if we do, we need to have a good reason for it. Testing could be a good reason, but in this case the same test could be run on createReader even though the abstraction is not ideal
| } | ||
| }; | ||
|
|
||
| PTF_TEST_CASE(TestFileFormatDetector) |
There was a problem hiding this comment.
Please see my previous comment. Maybe we can create a temp fake file with the expected data and run createReader()
Tests/Pcap++Test/PcapExamples/file_heuristics/pcapng-with-pcap-ext.pcapng.pcap
Outdated
Show resolved
Hide resolved
…in `createReader` instead of having the Format detector assume that is what is intended.
…t detector from libpcap behaviour.
3 participants
The PR adds heuristics based on the file content that is more robust than deciding based on the file extension.
The new decision model scans the start of the file for its magic number signature. It then compares it to the signatures of supported file types [1] and constructs a reader instance based on the result.
A new function
createReaderandtryCreateReaderhas been added due to changes in the public API of the factory.The functions differ in the error handling scheme, as
createReaderthrows andtryCreateReaderreturnsnullptron error.Method behaviour changes during erroneous scenarios:
getReadercreateReadertryCreateReadernullptrPcapFileDeviceReadernullptr