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

[no squash] Networking improvements / hardening #14217

Merged
merged 13 commits into from Jan 17, 2024
Merged

Conversation

sfan5
Copy link
Member

@sfan5 sfan5 commented Jan 5, 2024

key points:

  • resistance to flood and/or resource exhaustion (partial)
  • don't be a good target for UDP amplification
  • resistance to blind injection attacks and/or spoofed source IP

To do

This PR is Ready for Review.

It's best to review the commits individually.

How to test

I verified all the specialized stuff by hand. What this needs is real-world testing to catch oversights.

@sfan5 sfan5 added @ Network Bugfix 🐛 PRs that fix a bug labels Jan 5, 2024
src/network/connection.cpp Outdated Show resolved Hide resolved
src/network/connection.h Show resolved Hide resolved
src/network/connection.h Show resolved Hide resolved
src/network/connectionthreads.cpp Outdated Show resolved Hide resolved
@SmallJoker
Copy link
Member

I gave this PR some testing by joining several public servers. No issues found. It would be more interesting to see how well it works on a busy. Any server owner volunteers?

@lhofhansl
Copy link
Contributor

FWIW. I tried locally last weekend and did not observe any problems. This is of course a limited sample.
Maybe next weekend I can try with injecting some latency into the Linux networking stack.

@LoneWolfHT
Copy link
Contributor

LoneWolfHT commented Jan 16, 2024

This has been running on CTF for a week+ now (using the commits before cb5ac4d).
It works, and there are no issues as far as I can tell.

Will this cause/get put in a patch release when merged?

@sfan5
Copy link
Member Author

sfan5 commented Jan 16, 2024

Will this cause/get put in a patch release when merged?

Unlikely.

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

The code looks clean and is reliable based on the server testing results. 👍

@sfan5 sfan5 merged commit e8008c1 into minetest:master Jan 17, 2024
13 checks passed
@sfan5 sfan5 deleted the conn branch January 17, 2024 19:06
@Bastrabun
Copy link

Bastrabun commented Jan 17, 2024

Will this cause/get put in a patch release when merged?

Given that this was already used in the wild: Yes, please. Unless you're really going to fast track 5.9.0

Also, if my server can help test stuff, please ping me.

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.

None yet

5 participants