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

Major rewrite of the socket processing routines #1396

Merged
merged 2 commits into from
Aug 3, 2022

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Jul 31, 2022

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.

How familiar are you with the codebase?:

10


Major rewrite of the socket processing routines of FTL's Telnet-like API.

Before, we launched a new thread for each incoming connection (more than 40 threads concurrent were never allowed) to handle incoming connections independent from another and serve content concurrently. However, this could potentially be used to DoS FTL by opening and closing telnet sessions in very quick succession.
The new behavior is to, instead, launch five (compile-time setting) threads per type (independent for telnet over IPv4, IPv6, and Unix socket communication = 15 threads in total). Each of them accepts incoming connections in a FIFO manner (they always pick the first connection in the waiting connection queue). If too many requests are sent to FTL at once, they will simply be queued by the system until FTL is ready to accept them.

This is meant as a bugfix related to pi-hole/PADD#252,

…a new thread for each incoming connection (more than 40 threads were never allowed). This could potentially be used to DoS FTL by opening and closing telnet sessions in very quick succession. The new behavior is to, instead, launch five (compile-time setting) threads per type (5 for telnet IPv4, 5 for telnet IPv6, and 5 for Unix socket communication) and let them handle incoming connections in a FIFO manner. If too many requests are sent to FTL at once, they will simply have to wait until they are accepted.

Signed-off-by: DL6ER <dl6er@dl6er.de>
@DL6ER DL6ER requested a review from yubiuser July 31, 2022 07:06
@DL6ER DL6ER added the PR: Approval Required Open Pull Request, needs approval label Jul 31, 2022
Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

Why is istelnet missing in /api/reques.c for all messages >cacheinfo up to >interfaces

@DL6ER
Copy link
Member Author

DL6ER commented Jul 31, 2022

The Unix socket interface was introduced after the Telnet was already present. It was supposed to become the interface between FTL and the at that time drafted (and now abandoned) HTTP API written in another language. As the Telnet-like replying using ASCII-encoded numbers is pretty inefficient for machine-machine communication, we implemented MessagePack instead (#75) which allows for more efficient streaming.

After it got clear that the HTTP API isn't going to make it, we abandoned the development of the Unix socket part and new API endpoints did not get a different output than "ordinary" Telnet requests (this is the answer to your qeustion). We could (and maybe should) have removed the Unix socket sometime in between as I'm not sure anyone on this planet is actually using it. However, we still have it. All this code is removed for new/http where we will only provide a standardized REST interface instead.

@yubiuser
Copy link
Member

We could (and maybe should) have removed the Unix socket sometime in between as I'm not sure anyone on this planet is actually using it.

Maybe this PR is a good place to remove all fragments of the Unix socket?

@dschaper dschaper self-assigned this Jul 31, 2022
@dschaper
Copy link
Member

dschaper commented Aug 3, 2022

My self-assignment isn't a blocker for merge.

@DL6ER DL6ER merged commit 7ec4b58 into development Aug 3, 2022
@DL6ER DL6ER deleted the fix/socket_simplification branch August 3, 2022 15:57
@yubiuser yubiuser restored the fix/socket_simplification branch August 17, 2022 20:29
This was referenced Aug 17, 2022
@yubiuser yubiuser deleted the fix/socket_simplification branch August 22, 2022 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Bugfix PR: Approval Required Open Pull Request, needs approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants