Skip to content
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

support rtp/rtcp over tcp #2457

Merged
merged 1 commit into from
May 28, 2024
Merged

support rtp/rtcp over tcp #2457

merged 1 commit into from
May 28, 2024

Conversation

mmaatuq
Copy link
Contributor

@mmaatuq mmaatuq commented May 24, 2024

Please sign (check) the below before submitting the Pull Request:

Link to the related issue:

Describe changes:

  • support rtp/rtcp over tcp as per rfc4571.
  • will be in draft for now so i can add test case and fix other tests.

@mmaatuq mmaatuq marked this pull request as draft May 24, 2024 19:37
@mmaatuq mmaatuq changed the title support rtp/rtcp over tcp (#2422) support rtp/rtcp over tcp[WIP] May 24, 2024
@mmaatuq mmaatuq force-pushed the rtp_tcp_rfc_4571 branch 3 times, most recently from 3a54fd3 to 56f1dc1 Compare May 25, 2024 21:31
@mmaatuq mmaatuq changed the title support rtp/rtcp over tcp[WIP] support rtp/rtcp over tcp May 26, 2024
@mmaatuq mmaatuq marked this pull request as ready for review May 26, 2024 09:14
@mmaatuq
Copy link
Contributor Author

mmaatuq commented May 26, 2024

regarding

  /* NDPI_PROTOCOL_RTP */
   u_int32_t rtp_stage:2;
   u_int8_t rtp_seq_set[2];
   u_int16_t rtp_seq[2];

   /* NDPI_PROTOCOL_RTCP */ 
   u_int32_t rtcp_stage:2;

I'm not so sure if declaring them direct inside ndpi_flow struct is the right place to define them
I tried to make them part of the protos union,
but this caused some issues discovered during fuzz testing,
for example when we have rtp_stage = 1
but after this the protocol identified as tls,quic
and it doesn't have server_names but as rtp_stage is set
the below check passed

      if(flow->protos.tls_quic.server_names)
	ndpi_free(flow->protos.tls_quic.server_names);

which caused segfault.

also, i'm not completely sure if that was the reason for trying to free an object that wasn't allocated, the above explanation is my analysis.

Copy link
Collaborator

@IvanNardi IvanNardi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall it is quite good!
Just some minor issues below.
Furthermore:

  • try to merge together the 2 RTP traces, if possible
  • rebase

src/include/ndpi_typedefs.h Outdated Show resolved Hide resolved
src/include/ndpi_typedefs.h Outdated Show resolved Hide resolved
src/include/ndpi_typedefs.h Outdated Show resolved Hide resolved
src/lib/protocols/rtp.c Outdated Show resolved Hide resolved
@IvanNardi
Copy link
Collaborator

flow->protos

You got it right: you can write to flow->protos union only after set a proper classification

@mmaatuq mmaatuq force-pushed the rtp_tcp_rfc_4571 branch 2 times, most recently from 4c5e83c to 3cc92ec Compare May 27, 2024 19:43
@mmaatuq
Copy link
Contributor Author

mmaatuq commented May 27, 2024

Overall it is quite good! Just some minor issues below. Furthermore:

* try to merge together the 2 RTP traces, if possible

* rebase

for merging 2 rtp traces, here there are 3 traces 2 of them have the same rtp flow and the third one has fragmented packets (that are not covered in this PR)

@IvanNardi
Copy link
Collaborator

Overall it is quite good! Just some minor issues below. Furthermore:

* try to merge together the 2 RTP traces, if possible

* rebase

for merging 2 rtp traces, here there are 3 traces 2 of them have the same rtp flow and the third one has fragmented packets (that are not covered in this PR)

@mmaatuq , sorry, I have not been clearer: I was asking to merge "rtp_tcp.pcapng" that you added to the PR and the existing "rtp.pcapng" already in the repository. This way we have only one RTP trace in the tests directory...

* support rtp/rtcp over tcp as per rfc4571.

Signed-off-by: mmaatuq <[email protected]>
Copy link

sonarcloud bot commented May 28, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Collaborator

@IvanNardi IvanNardi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmaatuq, thanks for this work

@IvanNardi IvanNardi merged commit 6127e04 into ntop:dev May 28, 2024
35 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants