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

THRIFT-5779: Fix Thrift server getting killed for incomplete requests #2964

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

anshulmgupta
Copy link

The thrift server is getting killed when using security port scan tools in the hosts running the thrift server. These tools try to connect to the open ports by sending requests to the ports, and the error can happen when accept syscall call, waiting for an incoming connection, or receiving a connection that terminates before the accept process completes, hence killing the thrift server. This can cause potential DoS (Denial of service) attacks on the applications running the thrift server, causing them to become unresponsive. Sometimes, even just running the netcat (nc -zvvvw2 ) on the port remote can kill the entire thrift server, making it unresponsive.

@emmenlau
Copy link
Member

emmenlau commented Apr 24, 2024

I think this is a valid request. But it needs more discussion. Probably this should be moved to Jira.

General Context

Currently the Thrift server is implemented such that it will end on all server-side exceptions. This is not only in the case you found, there are other cases that may trigger the server to end.

In my humble opinion, the server should never end. I assume that an action, that triggers an exception in the server, is the "fault" of the client. In my understanding, the correct receiver of the problem is the client, while the server should resume its "normal" operation. The exception should therefore be passed down to the client.

For myself, I have modified the server to act in this way. Basically, I wrap the full server in the relevant try-catch-clocks, and pass all possible exceptions to the client.

However this changes a fundamental design of Thrift. While I would argue that this is an improvement for any user, it still requires discussion.

What do others think?

@Jens-G Jens-G added the c++ label May 1, 2024
@anshulmgupta
Copy link
Author

@emmenlau, I added the comment on JIRA. Can you please take a look? Thanks!

// before the accept process completes, hence killing the thrift serve
GlobalOutput.perror("TServerSocket::acceptImpl() ::accept() Retrying ",
errno_copy);
goto try_again;
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally goto should be avoided: ES.76: Avoid goto

This looks like it can be rewritten with a simple for loop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants