Skip to content

#1754 Add Modbus Support #1823

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.

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

Draft
wants to merge 25 commits into
base: dev
Choose a base branch
from
Draft

Conversation

yahyayozo
Copy link

  • Add ModbusLayer first implementation with header parsing only (no test included)

@yahyayozo yahyayozo requested a review from seladb as a code owner May 18, 2025 18:15
@yahyayozo yahyayozo marked this pull request as draft May 18, 2025 18:16
Copy link

codecov bot commented May 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.00%. Comparing base (a87c74a) to head (c8bd099).

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1823      +/-   ##
==========================================
+ Coverage   82.96%   83.00%   +0.03%     
==========================================
  Files         285      288       +3     
  Lines       49063    49173     +110     
  Branches    10599    10607       +8     
==========================================
+ Hits        40707    40815     +108     
- Misses       7196     7234      +38     
+ Partials     1160     1124      -36     
Flag Coverage Δ
alpine320 75.08% <92.98%> (+0.03%) ⬆️
fedora42 75.18% <93.10%> (+0.06%) ⬆️
macos-13 80.51% <98.86%> (+0.03%) ⬆️
macos-14 80.51% <98.86%> (+0.03%) ⬆️
macos-15 80.49% <98.86%> (+0.03%) ⬆️
mingw32 71.30% <74.46%> (-0.01%) ⬇️
mingw64 71.26% <74.46%> (-0.03%) ⬇️
npcap 84.95% <100.00%> (+0.02%) ⬆️
rhel94 74.95% <93.10%> (+0.03%) ⬆️
ubuntu2004 58.69% <79.03%> (+0.06%) ⬆️
ubuntu2004-zstd 58.81% <79.03%> (+0.07%) ⬆️
ubuntu2204 74.87% <92.98%> (+0.03%) ⬆️
ubuntu2204-icpx 61.48% <83.92%> (+0.02%) ⬆️
ubuntu2404 75.14% <92.98%> (+0.04%) ⬆️
ubuntu2404-arm64 75.12% <92.98%> (+0.03%) ⬆️
unittest 83.00% <100.00%> (+0.03%) ⬆️
windows-2019 84.98% <100.00%> (+0.03%) ⬆️
windows-2022 84.99% <100.00%> (+0.03%) ⬆️
winpcap 85.13% <100.00%> (+0.04%) ⬆️
xdp 50.58% <92.98%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Dimi1010 Dimi1010 linked an issue May 19, 2025 that may be closed by this pull request
@yahyayozo yahyayozo marked this pull request as ready for review May 21, 2025 20:13
@yahyayozo
Copy link
Author

yahyayozo commented May 21, 2025

@seladb I've prepared only 1 constructor which initiates the layer with default fields values
Can you please mention what are the other constructors I need to implement? also please mention if I need to follow on other protocols layers?

@seladb
Copy link
Owner

seladb commented May 22, 2025

@yahyayozo we usually have at least 2 c'tors, sometimes more:

  1. As part of the packet parsing flow. For example:
    MplsLayer(uint8_t* data, size_t dataLen, Layer* prevLayer, Packet* packet)
    which is used here (and in some other places also):
    m_NextLayer = new MplsLayer(payload, payloadLen, this, m_Packet);
  2. When the user wants to create a new layer of this type. This is the c'tor you already implemented, but usually we don't implement (only) c'tors with zero arguments. We tend to implement c'tors that allow the user to build the layer. In your case, I'd add arguments like transactionId, unitId and functionCode

@yahyayozo
Copy link
Author

Hi @seladb can you please guide me briefly on how to add tests for my ModbusLayer for parsing/crafting headers? As I want to pass to function types request/response after testing the header part.

@seladb
Copy link
Owner

seladb commented May 25, 2025

@yahyayozo sure! You can learn about tests here: https://pcapplusplus.github.io/docs/tests

At a high level:

  • You should add your tests under Tests/Packet++Test/Tests (don't forget to add your .cpp file to CMakeFiles.txt)
  • The tests use a homegrown testing framework. A test case is PTF_TEST_CASE. You can write 3 of those: for parsing, crafting, and editing
  • You can find the possible assertions here, the most common one is PTF_ASSERT_EQUAL(actual, expected)
  • You need to add your test cases to https://github.com/seladb/PcapPlusPlus/blob/master/Tests/Packet%2B%2BTest/main.cpp and add a PTF_RUN_TEST for each test case
  • When you build the tests, you should run them from the Tests/Packet++Test directory, like this: Bin/Packet++Test -t <YOUR_TEST_CASE>

Please look at similar tests that were written for other layers and follow the same patterns.

Please let me know if you have any questions.

@yahyayozo
Copy link
Author

@seladb I'll add tests for the headers crafting/parsing implementation and then you can a review before we move to adding the different PDU types

@yahyayozo yahyayozo marked this pull request as draft May 28, 2025 22:00
@yahyayozo
Copy link
Author

@seladb @Dimi1010 I've started adding some tests, but I have a question regarding the ".dat" files; how can I generate examples for some Modbus frames?

@seladb
Copy link
Owner

seladb commented Jun 2, 2025

@seladb @Dimi1010 I've started adding some tests, but I have a question regarding the ".dat" files; how can I generate examples for some Modbus frames?

If you open the file in Wireshark, you can choose a packet and right click -> Copy -> ... as a Hex Stream:

image

@seladb
Copy link
Owner

seladb commented Jun 12, 2025

@yahyayozo CI fails, can you please look into it? 🙁

@yahyayozo
Copy link
Author

@yahyayozo CI fails, can you please look into it? 🙁

@yahyayozo yahyayozo closed this Jun 12, 2025
@yahyayozo
Copy link
Author

@seladb I'll check further

@yahyayozo yahyayozo reopened this Jun 13, 2025
@yahyayozo
Copy link
Author

@seladb all the pre-commit checks have passed + the build was successful
I think if the pipeline build fails, I need to check my compiler, maybe I'm using a wrong version

@yahyayozo
Copy link
Author

@seladb CI passed! can you please make a review of the already done work before moving forward?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Modbus Protocol Support
3 participants