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

Split messages #582

Merged
merged 14 commits into from
Sep 12, 2020
Merged

Split messages #582

merged 14 commits into from
Sep 12, 2020

Conversation

corrados
Copy link
Contributor

Large Jamulus protocol messages may hit a limit where UDP packets are blocked by some fragmentation issues (see #255). Recently, when a lot of clients are connected to the server, we do not only see this UDP blocking issue for the server list but also at normal operation of Jamulus (i.e. the procol mechanism gets stuck), see #547.

Since the protocol stack in Jamulus is a very critical part regarding stability and security:

@softins Since you are the wireshark/protocol expert, could you please review the code to make sure it works correctly, does not break compatibility and does not crash.

@atsampson I saw that you have done a lot of stability/fuzzing testing for the Issue #314. Since with this new code a lot of the protocol mechanism is changed, I may have introduced new security issues. Could you please review the code and/or test the new code with your fuzzing tools to make sure it still is stable and secure?

src/protocol.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@pljones pljones left a comment

Choose a reason for hiding this comment

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

I can't see anything seriously wrong structurally. I can't comment on performance aspects just from looking at the code, though.

src/protocol.cpp Outdated Show resolved Hide resolved
src/protocol.cpp Outdated Show resolved Hide resolved
@softins
Copy link
Member

softins commented Sep 10, 2020

Sure, happy to have a look, but it will be on Friday.

@corrados
Copy link
Contributor Author

I can't see anything seriously wrong structurally. I can't comment on performance aspects just from looking at the code, though.

Thank you for your review.

@softins
Copy link
Member

softins commented Sep 11, 2020

It looks ok to me so far. I have compiled both server and client on local machines from the branch split_messages, with the split threshold reduced to 100 bytes so I can observe the split protocol without needing too many clients.

I have updated my Wireshark dissector at https://github.com/softins/jamulus-wireshark to display the new messages and the split container fields. I have yet to implement re-assembly of the split packet data parts into a virtual packet for display.

@softins
Copy link
Member

softins commented Sep 11, 2020

Maybe you also need to implement a CLM_REQ_SERVER_LIST_SPLIT, which a newer client could use instead of CLM_REQ_SERVER_LIST, to allow the server to split a CLM_CONN_CLIENTS_LIST in the same way? Although in that case, you might need a CLM_SPECIAL_SPLIT_MESSAGE container that doesn't need an ack response.

@corrados
Copy link
Contributor Author

split threshold reduced to 100 bytes so I can observe the split protocol without needing too many clients.

You can also simply use a long Chat text message. This will force the message split as well.

CLM_REQ_SERVER_LIST_SPLIT

Yes, something similar will be the next step. But first things first. Let's for now fix the problems with multiple clients.

@corrados
Copy link
Contributor Author

I have run this code on the Central server for days now and it seems to be stable. So I will merge this pull request to master now. You can still put your reviews here and I will consider them on the master branch.

@corrados corrados merged commit 1a3d265 into master Sep 12, 2020
@corrados corrados deleted the split_messages branch September 13, 2020 08:33
@atsampson
Copy link
Contributor

Could you please review the code and/or test the new code with your fuzzing tools to make sure it still is stable and secure?

I've had a look through these changes, and they look OK to me so far. I'll try and add support for these to my fuzzing branch when I get a chance.

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.

4 participants