Add Layer Build and Validation for DoIP (Diagnostic over IP) Support #1655
Conversation
|
As per the contributing guidelines, please retarget the PR to the |
|
Observed several issues in the CI pipelines, likely due to missing definitions for |
Packet++/header/ProtocolType.h
Outdated
| /** | ||
| * Diagnostic over IP protocol (DOIP) | ||
| */ | ||
| const ProtocolType DOIP = 38; |
There was a problem hiding this comment.
ID 38 is already taken by DHCPv6 protocol. The next available ID is 58, please add it after GTPv2
Packet++/header/HttpLayer.h
Outdated
| @@ -437,7 +437,6 @@ namespace pcpp | |||
|
|
|||
| HttpResponseStatusCode() = default; | |||
|
|
|||
| // cppcheck-suppress noExplicitConstructor | |||
There was a problem hiding this comment.
Why removing this cppcheck-suppress comment?
There was a problem hiding this comment.
Cppcheck flags this as an incorrect suppression on my local machine. Let me know if you think I should revert it.
There was a problem hiding this comment.
That is interesting. Which version of CPPcheck are you using?
There was a problem hiding this comment.
2.7 as recommanded in CONTRIBUTING.md and still flags incorrect suppression, I'm keeping this changes so can I commit my recents updates,
3rdParty/json/include/json.hpp
Outdated
There was a problem hiding this comment.
same thing as my previous comment, I only removed some suppress-checks detected as incorrect
There was a problem hiding this comment.
Can you add one pcap file with all of these packets?
There was a problem hiding this comment.
yes sure, I'll revert suppress-checks and add pcap file containig all tested packets in next PR, also try to cover some missing checks based on codecov feedback
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1655 +/- ##
==========================================
+ Coverage 83.10% 83.66% +0.55%
==========================================
Files 283 286 +3
Lines 48929 51108 +2179
Branches 10303 10988 +685
==========================================
+ Hits 40664 42759 +2095
- Misses 7113 7177 +64
- Partials 1152 1172 +20
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:
|
|
@raissi-oussema are you planning to continue working on this PR? |
|
Hi, I was engaged with other tasks, but I plan to get back to this PR soon. Thanks for your understanding! |
.improve maps searchs for doipEnumsToStrings .cover more uses cases based on codecov feedback
|
Design suggestions or code improvements are always welcome and greatly appreciated. |
|
@raissi-oussema to make it easier to review, do you think you can add some documentation on the DoIP protocol to the PR body? It'd mostly be helpful to get more details on the header structure and different possible message |
|
@seladb I need support for CI pipelines, I can't figure out why are they still failing. And a clear documentation was successfully added to PR body to make it easier for you to start the code review. |
Doxigen pipeline: XDP pipeline: VS pipeline: |
What could be the problem for dioxygen pipeline, doipLayer.h is well documented [line 42] ? |
Packet++/header/DoIpLayer.h
Outdated
| // implement abstract methods | ||
|
|
||
| /** | ||
| * TODO, parse UDS layer |
There was a problem hiding this comment.
Is this for another PR? If so, should the remaining data be parsed as a generic payload layer for now?
There was a problem hiding this comment.
m_NextLayer is intended to be the UDS layer, which has not been implemented yet. In the future, as more knowledge is gained, either I or another contributor may add this functionality. For now, I suggest parsing it as a generic layer, as you mentioned.
PS: the nextLayer will be parsed only when the payloadType is 0x8001.
There was a problem hiding this comment.
your feedback is highly appertiated, what do you think about adding this snippet of code:
void parseNextLayer() override
{
if (getPayloadType() == DoIpPayloadTypes::DIAGNOSTIC_MESSAGE_TYPE)
{
size_t headerLen = sizeof(doiphdr);
if (m_DataLen <= headerLen + 2 /*source address size*/ + 2 /*target address size*/)
return;
uint8_t* payload = m_Data + (headerLen + 2 + 2);
size_t payloadLen = m_DataLen - (headerLen + 2 + 2);
m_NextLayer = new PayloadLayer(payload, payloadLen, this, m_Packet);
return;
}
}`
There was a problem hiding this comment.
your feedback is highly appertiated, what do you think about adding this snippet of code:
void parseNextLayer() override { if (getPayloadType() == DoIpPayloadTypes::DIAGNOSTIC_MESSAGE_TYPE) { size_t headerLen = sizeof(doiphdr); if (m_DataLen <= headerLen + 2 /*source address size*/ + 2 /*target address size*/) return; uint8_t* payload = m_Data + (headerLen + 2 + 2); size_t payloadLen = m_DataLen - (headerLen + 2 + 2); m_NextLayer = new PayloadLayer(payload, payloadLen, this, m_Packet); return; } }`
Seems good. Can the source address and target address used in that type of message be accessed from this layer? Since we are excluding it from the generic payload.
A minor tip, having headerLen be marked as constexpr might allow some compiler optimizations (such as the arithmetic using it + a literal being computed during compilation and hardcoded if possible).
Also why the return statement that is at the end of the block anyway?
There was a problem hiding this comment.
@Dimi1010 , this approach appears both safer and cleaner. It allows us to construct a DiagnosticMessage directly from the current layer, providing direct access to the diagnostic data. Using this data, we can then build a generic PayloadLayer.
void parseNextLayer() override
{
DiagnosticMessageData diagnosticMessage;
if (diagnosticMessage.buildFromLayer(this))
{
m_NextLayer = new PayloadLayer(diagnosticMessage.diagnosticData.data(),
diagnosticMessage.diagnosticData.size(), this, m_Packet);
}
}
buildFromLayer safely parses the current layer and verifies whether it represents a valid diagnostic message.
what do you think ?
There was a problem hiding this comment.
Sure, but my question stands. DiagnosticMessageData has two other members (sourceAddress and targetAddress) which at the moment I don't see how the user can access them easily. They have neither accessors in the current DoIPLayer or are included as part of the UDSLayer (currently PayloadLayer).
There was a problem hiding this comment.
sourceAddress and targetAddress are public members of DiagnosticMessageData class just like diagnosticData and they are not part of the UDS layer:
uint16_t sourceAddress; /**< Source address of the message. */
uint16_t targetAddress; /**< Target address for the diagnostic message. */
std::vector<uint8_t> diagnosticData; /**< Diagnostic message data with dynamic length. */
the user can access these fields by the method buildFromLayer .
I've made this dummy function just to show how to access to these fields :
void DoIpLayer::resolveDiagMessageFields()
{
DiagnosticMessageData diagnosticMessage;
if (diagnosticMessage.buildFromLayer(this))
{
uint16_t srcAddr = diagnosticMessage.sourceAddress;
uint16_t targetAddr = diagnosticMessage.targetAddress;
std::vector<uint8_t> diagData = diagnosticMessage.diagnosticData;
}
}
Do you think this implementation need more improvement ?
There was a problem hiding this comment.
Ok, if i am understanding correctly the sequence is this?
- User somehow receives a
DoIPLayerfrom aPacket - User checks the payload type (via
DoIPLayer::getPayloadType()) - Depending on the payload type user uses
T::buildFromLayer(DoIPLayer)(T being the corresponding message struct) to populate the data from the layer into the struct.
Am I understanding the flow correctly?
There was a problem hiding this comment.
yes you are absolutely correct.
There was a problem hiding this comment.
Sure, that works fine. 👍
Co-authored-by: Dimitar Krastev <[email protected]>
Packet++/src/DoIpLayer.cpp
Outdated
| DiagnosticMessageData diagnosticMessage; | ||
| if (diagnosticMessage.buildFromLayer(*this)) | ||
| { | ||
| // handle UDS layer as generic PayloadLayer for now. | ||
| m_NextLayer = new PayloadLayer(diagnosticMessage.diagnosticData.data(), | ||
| diagnosticMessage.diagnosticData.size(), this, m_Packet); |
There was a problem hiding this comment.
parseNextLayer shouldn't generate new layers, it should just parse them if they exist. In this case since the next layer is always a generic PayloadLayer, it should get the data beyond the header
There was a problem hiding this comment.
This is exactly what we're doing here: creating a diagnostic message is to validate if we are dealing with a valid diagnostic message using buildFromLayer, data then are passed generic PayloadLayer. The only case where we need this nextLayer is when the payload type is a diagnostic message
you can refer to this comment for more details.
There was a problem hiding this comment.
What if diagnosticMessage.buildFromLayer() returns false? We should still parse the rest of the data which is also a PayloadLayer. Since we don't parse a more specific layer, using DiagnosticMessageData is not needed here
There was a problem hiding this comment.
If diagnosticMessage.buildFromLayer() returns false, it means there is no UDS layer and no need to parse any additional data and m_NextLayer would remain unset.
So if I understand you correctly, you’re suggesting that we should always generate a PayloadLayer containing the data beyond the header, regardless of the payload type?
If that’s the case, the UDS layer should only be represented when the payload type is DoIpPayloadTypes::DIAGNOSTIC_MESSAGE_TYPE.
I'm not entirely sure whether defining m_NextLayer for all payload types is necessary. Could this lead to any misinterpretation or incorrect handling of certain payload types?
For example, if we have a DoIP message with the type DoIpPayloadTypes::DIAGNOSTIC_MESSAGE_NEG_ACK and m_NextLayer is not nullptr, it would be misleading since this type does not have any additional layers (DoIP is the final layer like all other type except DoIpPayloadTypes::DIAGNOSTIC_MESSAGE_TYPE ).
If I’ve misunderstood how parseNextLayer works, I'm open to discuss it further.
There was a problem hiding this comment.
The way packet parsing works is that a layer should always parse the next layer if there's additional data left in the buffer. If the layer knows what the next layer should be - it should create that layer, otherwise it should create a PayloadLayer which is another way of saying "there is some data left in the buffer but I don't know what it is".
There was a problem hiding this comment.
This comment was not yet addressed - parseNextLayer should always create a layer (PayloadLayer or other) from the remaining data
There was a problem hiding this comment.
My last update was :
void DoIpLayer::parseNextLayer()
{
if (getPayloadType() == DoIpPayloadTypes::DIAGNOSTIC_MESSAGE_TYPE)
{
size_t headerLen = sizeof(doiphdr);
if (m_DataLen <= headerLen + 2 /*source address size*/ + 2 /*target address size*/)
return;
uint8_t* payload = m_Data + (headerLen + 2 + 2);
size_t payloadLen = m_DataLen - (headerLen + 2 + 2);
m_NextLayer = new PayloadLayer(payload, payloadLen, this, m_Packet);
}
}
like first implementation with @Dimi1010 ,
the only case that we have remaining data to be parsed is when we have this specific type DoIpPayloadTypes::DIAGNOSTIC_MESSAGE_TYPE, else we are dealing with an invalid doip packet,
@seladb , @Dimi1010 , what do you think ?
There was a problem hiding this comment.
feel free to provide a corrected version of the code if you think I missed anything.
There was a problem hiding this comment.
@raissi-oussema just to make sure I understand: if the message isn't of type DoIpPayloadTypes::DIAGNOSTIC_MESSAGE_TYPE) then there is no layer after the DoIP message (meaning the DoIP message is the last message in the packet)?
. Move DoIpEnumToStringPayloadType, DoIpEnumToStringProtocolVersion maps to DoIpLayer.cpp . Remove serializeData (...) funct and use memcpy . Make buildLayer private . Improve getPayloadType() func . Add test for invalid paylaodType . Reduce toString() presentation
Packet++/src/DoIpLayer.cpp
Outdated
| DiagnosticMessageData diagnosticMessage; | ||
| if (diagnosticMessage.buildFromLayer(*this)) | ||
| { | ||
| // handle UDS layer as generic PayloadLayer for now. | ||
| m_NextLayer = new PayloadLayer(diagnosticMessage.diagnosticData.data(), | ||
| diagnosticMessage.diagnosticData.size(), this, m_Packet); |
There was a problem hiding this comment.
This comment was not yet addressed - parseNextLayer should always create a layer (PayloadLayer or other) from the remaining data
|
@Dimi1010 , could you please support, what could be the reason of failed pipeline here ? |
from the doxygen pipeline |
Packet++/header/DoIpLayerData.h
Outdated
| /// - `activationType`: The type of activation requested. | ||
| /// - `reservedIso`: Reserved bytes as defined by ISO specifications. | ||
| /// - `reservedOem`: Reserved bytes as defined by OEM specifications, only if present. | ||
| bool buildFromLayer(const DoIpLayer& doipLayer); |
There was a problem hiding this comment.
I started reviewing DoIpLayerData.h, and one of the first things I looked for was how a user can get the DoIP data (meaning one of these classes that inherit from IDoIpMessageData). From what I understand the user needs to do something like this:
pcpp::DoIpLayer* doipLayer = myPacket.getLayerOfType<pcpp::DoIpLayer>();
if (doipLayer->getPayloadType() == pcpp::DoIpPayloadTypes::GENERIC_HEADER_NEG_ACK)
{
pcpp::GenericHeaderNackData data;
if (!data.buildFromLayer(*doipLayer))
{
throw std::invalid_argument("Invalid GENERIC_HEADER_NEG_ACK message");
}
// do something with `data`...
}
else
{
// this is not a GENERIC_HEADER_NEG_ACK message
}While this approach can work, I think it's not very clear for users. My understanding is that a DoIP message has the following structure:
- 8 first bytes: fixed header of type
doiphdr - Additional payload with size of 0 or more, according to the nessagetype
- In some messages (probably only
Diagnostic message (0x8001)?) - there might be additional bytes that we can consider anotherPayloadLayer
What I suggest is a different structure that is a bit similar to what we do in BgpLayer:
class DoIpLayer : Layer
{
// should have a protected constructor - this class is abstract and cannot be instantiated
// contains most of the getters/setters it currently has
// Takes raw data and creates one of the child classes of DoIpLayer, according to the payload type
static DoIpLayer * parseDoIpLayer(uint8_t* data, size_t dataLen, Layer* prevLayer, Packet* packet);
}
class RoutingActivationRequest : public DoIpLayer
{
// Should have a public constructor that takes the relevant params and creates the layer fields (sourceAddress, activationType, etc...)
// getters and setters
uint16_t getSourceAddress();
void setSourceAddress(uint16_t value);
DoIpActivationTypes getActivationType();
void setActivationType(DoIpActivationTypes activationType);
...
}
class RoutingActivationResponse: public DoIpLayer
{
// Should have a public constructor that takes the relevant params and creates the layer fields (sourceAddress, respondCode, etc...)
// getters and setters
uint16_t getSourceAddress();
void setSourceAddress(uint16_t value);
DoIpRoutingResponseCodes getResponseCode();
void setResponseCode(DoIpRoutingResponseCodes responseCode);
...
}
class GenericHeaderNack: public DoIpLayer
{
// Should have a public constructor that takes the relevant params and creates the layer fields (genericNackCode)
// getters and setters
DoIpGenericHeaderNackCodes getGenericNackCode();
void setGenericNackCode(DoIpGenericHeaderNackCodes code);
}
// do the same for the other classes that currently inherit from `IDoIpMessageData`
...
...
// in UdpLayer.cpp
void UdpLayer::parseNextLayer()
{
...
else if ((DoIpLayer::isDoIpPort(portSrc) || DoIpLayer::isDoIpPort(portDst)) &&
(DoIpLayer::isDataValid(udpData, udpDataLen)))
m_NextLayer = DoIpLayer::parseDoIpLayer(udpData, udpDataLen, this, m_Packet);
...
}
If you do that the user can use the DoIP layer as follows:
auto genericHeaderNack = myPacket.getLayerOfType<pcpp::GenericHeaderNack>();
if (genericHeaderNack != nullptr)
{
// do something with `genericHeaderNack`...
}I think this is a simpler implementation of the layer which is simpler for users and is more aligned with other layers we already have (like BgpLayer, but there are others too).
There was a problem hiding this comment.
Yes sure, I'll proceed for the new solution if you feel it is more aligned with other layers implementation.
So all classes in doipLayerData.cpp will be modified as suggested and moved to doipLayer.cpp file.
Is that correct ?
There was a problem hiding this comment.
Yes, I think we can move all classes and enums to DoIpLayer.h and DoIpLayer.cpp and remove DoIpLayerData.h and DoIpLayerData.cpp
.Remove doipLayerData.cpp and doipLayerData.h .Move all enum classes to doipLayer.h .Move string maps to doipLayer.cpp .Change desidgn structure to : .All derived classes for all doipPayloadTypes are inherited from doipLayer itself witch is an abstract interface with protected constructor .Change isLayerDataValid, isProtocolVersionValid, isPayloadTypeValid, to a static func to validate layer from .Make getters and setters for all derived classes and improve dissecting and crafting logic. .Enhance logging errors
|
@raissi-oussema I triggered CI and some tests fail, can you please check? |
Packet++/header/DoIpLayer.h
Outdated
| /// @brief Length of the External Identifier (EID) field. | ||
| #define DOIP_EID_LEN 6 | ||
|
|
||
| /// @brief Length of the Group Identifier (GID) field. | ||
| #define DOIP_GID_LEN 6 | ||
|
|
||
| /// @brief Length of the Vehicle Identification Number (VIN) field. | ||
| #define DOIP_VIN_LEN 17 | ||
|
|
||
| /// @brief Length of the Reserved ISO field. | ||
| #define DOIP_RESERVED_ISO_LEN 4 | ||
|
|
||
| /// @brief Length of the Reserved OEM field. | ||
| #define DOIP_RESERVED_OEM_LEN 4 |
There was a problem hiding this comment.
Nit: Can we use constexpr here instead of macros?
NB: Might need to be inline constexpr since they are in header.
There was a problem hiding this comment.
@Dimi1010 ,I skimmed through this topic and noticed that inline constexpr is only available since C++17.
Packet++/header/DoIpLayer.h
Outdated
| DoIpProtocolVersion parsedVersion = static_cast<DoIpProtocolVersion>(version); | ||
|
|
||
| switch (parsedVersion) | ||
| { | ||
| case DoIpProtocolVersion::DefaultVersion: | ||
| case DoIpProtocolVersion::ReservedVersion: | ||
| case DoIpProtocolVersion::Version01Iso2010: | ||
| case DoIpProtocolVersion::Version02Iso2012: | ||
| case DoIpProtocolVersion::Version03Iso2019: | ||
| case DoIpProtocolVersion::Version04Iso2019_AMD1: | ||
| if (parsedVersion == DoIpProtocolVersion::UnknownVersion || | ||
| parsedVersion == DoIpProtocolVersion::ReservedVersion || | ||
| (parsedVersion == DoIpProtocolVersion::DefaultVersion && | ||
| type != DoIpPayloadTypes::VEHICLE_IDENTIFICATION_REQUEST_WITH_VIN && | ||
| type != DoIpPayloadTypes::VEHICLE_IDENTIFICATION_REQUEST_WITH_EID && | ||
| type != DoIpPayloadTypes::VEHICLE_IDENTIFICATION_REQUEST)) |
There was a problem hiding this comment.
Nit: parsedVersion can't be UnknownVersion in the if statement. There is no case for it.
Nit: I feel like it would be cleaner to have the reserved version fail in the reserved case, instead of combining all the tests in one big if statement.
Also, having the default version case after the reserved case in the switch order can simplify the if by checking only for the type inside the DefaultVersion case and letting the control flow fall through to the following cases if the type is valid.
(Unsure if we have warnings for fall though cases tho, tho)
Example:
case ReservedVersion:
{
// Error message here.
return false;
}
case DefaultVersion:
{
if(type != DoIpPayloadTypes::VEHICLE_IDENTIFICATION_REQUEST_WITH_VIN &&
type != DoIpPayloadTypes::VEHICLE_IDENTIFICATION_REQUEST_WITH_EID &&
type != DoIpPayloadTypes::VEHICLE_IDENTIFICATION_REQUEST)
{
// Error message here.
return false;
}
// Notice no break statement. Control flow falls through.
}
case ...:
case ...:
case ...:
/* Rest of the parsing code */
There was a problem hiding this comment.
Yes I agree with you, I have enhanced the check to :
inline bool DoIpLayer::isProtocolVersionValid(uint8_t version, uint8_t inVersion, DoIpPayloadTypes type)
{
DoIpProtocolVersion parsedVersion = static_cast<DoIpProtocolVersion>(version);
switch (parsedVersion)
{
case DoIpProtocolVersion::ReservedVersion:
{
PCPP_LOG_ERROR("[Malformed doip packet]: Reserved ISO DoIP protocol version detected: 0x"
<< std::hex << static_cast<int>(version));
return false;
}
case DoIpProtocolVersion::DefaultVersion:
if (type != DoIpPayloadTypes::VEHICLE_IDENTIFICATION_REQUEST_WITH_VIN &&
type != DoIpPayloadTypes::VEHICLE_IDENTIFICATION_REQUEST_WITH_EID &&
type != DoIpPayloadTypes::VEHICLE_IDENTIFICATION_REQUEST)
{
PCPP_LOG_ERROR("[Malformed doip packet]: Invalid/unsupported DoIP version!");
return false;
}
case DoIpProtocolVersion::Version01Iso2010:
case DoIpProtocolVersion::Version02Iso2012:
case DoIpProtocolVersion::Version03Iso2019:
case DoIpProtocolVersion::Version04Iso2019_AMD1:
{
if (version != static_cast<uint8_t>(~inVersion))
{
PCPP_LOG_ERROR("[Malformed doip packet]: Protocol version and inverse version mismatch! Version: 0x"
<< std::hex << static_cast<int>(version) << ", Inverted: 0x"
<< static_cast<int>(inVersion));
return false;
}
return true;
}
default:
PCPP_LOG_ERROR("[Malformed doip packet]: Unknown DoIP protocol version: 0x" << std::hex
<< static_cast<int>(version));
return false;
}
}
looks much cleaner.
…Ack and diagnosticNac
|
@seladb, could you please have a look, I've made serval changes in this current version and layer now is more aligned with other layer implementation, you can start reviewing this PR, |
DoIP Protocol Overview
The Diagnostic over IP (DoIP) protocol is used in automotive diagnostic systems to facilitate communication between diagnostic tools and ECUs (Electronic Control Units) over IP-based networks. It enables remote diagnostics, configuration, and software updates over Ethernet, offering an efficient and scalable solution for modern vehicles.
Header Structure (8 bytes)
Pyload types / code / structure