Add comprehensive SCTP protocol support#2031
Conversation
jordanvrtanoski
force-pushed
the
sctp
branch
from
07854bb to
0844b5c
Compare
jordanvrtanoski
force-pushed
the
sctp
branch
from
0844b5c to
fb63b3b
Compare
Dimi1010
left a comment
Dimi1010
left a comment
There was a problem hiding this comment.
Thank you for the submission. :)
| /// @class SctpChunk | ||
| /// A wrapper class for SCTP chunks. This class does not create or modify chunk records, | ||
| /// but rather serves as a wrapper and provides useful methods for retrieving data from them | ||
| class SctpChunk |
There was a problem hiding this comment.
This is essentially a view class right?
Somewhat thinking out loud but can we split this class into several view subclasses for each chunk type?
Currently the class seems a bit unwieldy with all the different chunk type methods?
Can we instead have:
- SctpChunk - Common Methods (
getChunkType(), etc)- SctpDataChunk - A data chunk.
- SctpInitChunk - Init/InitAck chunk.
- ScptSackChunk - Sack chunk.
- ...
Since the chunk objects only hold a pointer, they can be treated as value objects, similar to std::string_view.
class SctpChunk
{
/* Generic chunk methods */
SctpChunkType getChunkType() const;
}
class SctpDataChunk : public SctpChunk {
SctpDataChunk(SctpChunk chunk) { /* validate correct chunk type */ }
/* Data specific chunk accessors */
}
// Other chunk types.The inheritance in the above example is mostly for code reuse of the common methods, not for polymorphism, therefor the lack of any virtual methods.
Do you think that would work or is there something I am missing?
There was a problem hiding this comment.
That's good observation, let me see what it takes to refactor it to be more aligned to OO principles.
There was a problem hiding this comment.
The reason the SctpChunk is designed as a monolith class in the first pace is to avoid the heap overhead that inheritance would create, as well as it's impact on the cache as the objects in the hierarchy have bad locality.
Having the chunks as views classes on the SctpChunk will address my original performance concerns, so I will refactor the code based on the suggestion.
|
Converted the PR to draft to address the proposed refactoring. |
There was a problem hiding this comment.
Can you please add the original pcap file used for these .dat files?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #2031 +/- ##
==========================================
+ Coverage 82.72% 82.99% +0.26%
==========================================
Files 332 335 +3
Lines 59807 64735 +4928
Branches 12591 13560 +969
==========================================
+ Hits 49475 53724 +4249
- Misses 8947 10138 +1191
+ Partials 1385 873 -512
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:
|
jordanvrtanoski
force-pushed
the
sctp
branch
5 times, most recently
from
15ee3a5 to
5a4d9c5
Compare
|
@jordanvrtanoski this PR is opened for a long time, are you planning to continue working on it? Otherwise we can close it for now |
|
I will rebase it and prepare for merge, as I see all concerns ware addressed. |
Thanks @jordanvrtanoski ! Can you please make sure CI passed as well? |
|
Sure |
jordanvrtanoski
force-pushed
the
sctp
branch
3 times, most recently
from
d0884b1 to
4415715
Compare
|
@seladb, I have problems in my github runner with actions/setup-java@v5 test. I am getting "Error: Java setup process failed due to: error code: 520". The project is build fine but the test is failing as the JDK can not be set up properly. Have you faced this issue? This is blocking Android tests, all other tests are green. Any tips how to proceed? |
|
After multiple retries, one of the Android test passed, but the other 3 are still failing with the same issue, I assume the repository had reached some type of limitation. |
|
After a few more tries, all the checks on my repo passed! |
Implement full SCTP (Stream Control Transmission Protocol) layer with support for: Core Protocol (RFC 9260): - All standard chunk types (DATA, INIT, INIT-ACK, SACK, HEARTBEAT, ABORT, SHUTDOWN, ERROR, COOKIE-ECHO, COOKIE-ACK, ECNE, CWR, SHUTDOWN-COMPLETE) - CRC32c checksum calculation and validation - Chunk and parameter action bits helpers - Bundling validation per RFC 9260 - Host Name Address deprecation detection Extensions: - AUTH chunk with HMAC-SHA1/SHA256 computation and verification (RFC 4895) - ASCONF/ASCONF-ACK for dynamic address reconfiguration (RFC 5061) - RE-CONFIG for stream reconfiguration (RFC 6525) - FORWARD-TSN for partial reliability (RFC 3758) - I-DATA and I-FORWARD-TSN for message interleaving (RFC 8260) - PAD chunk (RFC 4820) - NR-SACK experimental support (IANA registered, draft-based) - Zero Checksum Acceptable parameter (RFC 9653) Additional features: - Type-safe chunk views: Zero-overhead wrapper classes for all 24 chunk types, enabling type-safe field access without runtime overhead - Parameter iterators for INIT, RE-CONFIG, and ASCONF chunks - Error cause iterator for ABORT/ERROR chunks - Extended PPID enum with 70+ protocol identifiers from IANA registry - Chunk creation APIs for building SCTP packets Includes 91 test cases covering parsing, creation, and validation.
|
@seladb ready to merge |
There was a problem hiding this comment.
Well. There are some long files here. I did a first pass review. I will probably do another pass later, tho. I am really liking how the view classes turned out. I have one comment about the return type of pointers from them tho. The other things I looked at LGTM.
PS: @jordanvrtanoski if you make further edits to this, can you please not force push. It makes it really hard to find the edits since it deletes the old commits. It will all get squashed on merge, so it doesn't matter for the main branch, but it makes it easier to track changes during the review process.
| // Cannot add INIT, INIT-ACK, or SHUTDOWN-COMPLETE to a packet with existing chunks | ||
| if (chunkType == SctpChunkType::INIT || chunkType == SctpChunkType::INIT_ACK || | ||
| chunkType == SctpChunkType::SHUTDOWN_COMPLETE) | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
nit: Perhaps move this condition above the O(N) while loop?
| size_t getTotalSize() const; | ||
|
|
||
| /// @return Pointer to chunk value/payload (after the 4-byte header) | ||
| uint8_t* getValue() const; |
There was a problem hiding this comment.
Would it be better to return uint8_t const*?
Currently the returned buffer can be mutated, but I am not sure if that is intended behaviour.
PS: The question also applies for all other view types that return pointer to internals.
3 participants
Implement full SCTP (Stream Control Transmission Protocol) layer with support for:
Core Protocol (RFC 9260):
Extensions:
Additional features:
Includes 72 test cases covering parsing, creation, and validation.