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.
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...
With what the current iteration of the detector is, either works, tbh. The initial implementation, which was added to the .cpp because a lot of the business logic of the factory was intermixed with the detector. (e.g. Zst archive -> PcapNG folding, Pcap and PcapNano being just Pcap). That is no longer the case.
But sure, it was extracted due to the requirements for unit tests for every magic number, which would be much easier to maintain in the long run if done directly on the format detector. For one, it avoids the filesystem, which IMO is always more trouble than its worth if it can be avoided relatively trivially. For another, it is a technical debt on expanding createReader validation logic.
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
createReadereven though the abstraction is not ideal
The good reason I have is that this unit test through createReader will need to be changed literally in the next PR I plan to make after this one , to keep it running even though I don't plan to touch the format detector code.
The planned changes in validation being:
- Compressed PcapNG: Unpacking a ZST archive in
createReaderand checking the format of the archived file. This will essentially brick any spoofed ZST file, as it will not be able to be unpacked, fail factory validation and returnnullptr. - Have
open()/close()be called inside the factory prior to device return to run secondary validation that the reader can actually be opened. File devices can't be retargeted so no point in returning a reader that will just fail to open when the user tries, IMO. This will essentially brick all spoofed files since they can't be opened by the device by definition.
If you insist on having it done through createReader then fine, but that solution opens up more work for future changes that I have planned around with the current one.
There was a problem hiding this comment.
But sure, it was extracted due to the requirements for unit tests for every magic number, which would be much easier to maintain in the long run if done directly on the format detector. For one, it avoids the filesystem, which IMO is always more trouble than its worth if it can be avoided relatively trivially. For another, it is a technical debt on expanding
createReadervalidation logic.
I don't think this piece of logic will change much because I don't expect more file types to be added (and even if we will, it only happens rarely), so maintenance in the long run shouldn't be an issue. For the same reason I don't think we'll expand createReader much
- Compressed PcapNG: Unpacking a ZST archive in
createReaderand checking the format of the archived file. This will essentially brick any spoofed ZST file, as it will not be able to be unpacked, fail factory validation and returnnullptr.
I don't think we want to unpack the Zstd archive just to see if it's valid. We have the pacpng library that does that. The logic in createReader does an educated guess, not a bullet-proof validation. Otherwise we can argue that checking the magic numbers is not enough - why not validate the entire pcap / pcapng file? Of course we don't want to do that because libpcap is doing it for us. The same should apply for Zstd
- Have
open()/close()be called inside the factory prior to device return to run secondary validation that the reader can actually be opened. File devices can't be retargeted so no point in returning a reader that will just fail to open when the user tries, IMO. This will essentially brick all spoofed files since they can't be opened by the device by definition.
As mentioned earlier, I don't think it's the approach we want. createReader should do an educated guess, nothing more
If you insist on having it done through
createReaderthen fine, but that solution opens up more work for future changes that I have planned around with the current one.
Again, I don't think this logic will change much after this refactoring. Even if it will, I don't think it'll be a huge refactoring
There was a problem hiding this comment.
The logic in createReader does an educated guess, not a bullet-proof validation
Curious why wouldn't you want that?
I would reason that having a better validation sequence inside the factory would make for cleaner UX due to less boilerplate needed by the user?
auto dev = IFileDevice::tryCreateReader("filePath");
if (!dev)
{
// User has to handle error here.
}
// User has to open device.
bool res = dev.open();
if (!res)
{
// User also has to handle error here.
// Zst particularly may fail here if contents are not pcapng.
}
// Use device here.Having the integrated open / validation would allow single line, before use. I don't see much use cases where you would want to create a device reader and not open to read from it, no? It also fits with the RAII methodology of avoiding a 2-stage init where possible.
auto dev = IFileDevice::tryCreateReader("filePath");
if(!dev)
{
// Failed to create device.
// Note: No need for second boilerplate error handler prior to use.
}
// Use device here.The live devices need open() because they are created by the runtime at startup.
File devices don't need to have that limitation since they are entirely created by the user.
I don't think we want to unpack the Zstd archive just to see if it's valid. We have the pacpng library that does that.
Yes, but I am unsure if it gives a precise error message of what went wrong or just a generic failure error.
Otherwise we can argue that checking the magic numbers is not enough - why not validate the entire pcap / pcapng file?
Which is as simple as calling open() inside the factory function, no? As you said, the backend already does validation, so why not reuse it for the factory validation?
There was a problem hiding this comment.
Curious why wouldn't you want that?
Extracting the archived file just to verify the format is wasteful and might take a long time, especially if it's called on large files. Also - for these large files it'd mean the file is extracted twice - once in createReader and again when actually reading the file
Yes, but I am unsure if it gives a precise error message of what went wrong or just a generic failure error.
If this is indeed the case, maybe we need to fix the LightPcapNg code?
Which is as simple as calling
open()inside the factory function, no? As you said, the backend already does validation, so why not reuse it for the factory validation?
Not necessarily - as far as I know open() checks mostly the header and doesn't go over the rest of the file, so a user can open a file with a correct header but corrupted data and reading the file will fail
There was a problem hiding this comment.
Extracting the archived file just to verify the format is wasteful and might take a long time, especially if it's called on large files.
There is no need to extract the entire file. ZST compression works on independent frames allowing frame-by-frame (streaming) decompression. We only need to decompress the first frame to read the magic number to validate that the archive contents appear to be PcapNG. How large the total file is is irrelevant.
Incidentally, frame-by-frame is also how LightPcapNG reads the ZST archive. It decompresses a frame, reads the fully decompressed PcapNG records in it, and decompresses the next frame if needed.
If this is indeed the case, maybe we need to fix the LightPcapNg code?
Which is C code, and makes it much harder to output a readable error. Not to mention we need to deal with passing that error up the stack.
Not necessarily - as far as I know open() checks mostly the header and doesn't go over the rest of the file, so a user can open a file with a correct header but corrupted data and reading the file will fail
But it will still have passed open(). My idea isn't that createReader should validate that everything is correct. It is that it should validate just enough to guarantee that the returned device can successfully pass an open() call. The device might even be returned already opened and ready for reading, reducing the user side boilerplate.
There is no reason to return a device that can't even be opened, since the user can't do anything with it. It just adds more boilerplate as the user has to do the error handling twice.
If the records afterwards are corrupted at some point, the read should fail when the corrupted data is reached.
| } | ||
| }; | ||
|
|
||
| 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