issue: 3594311 Fix segfault for delegate incoming handshake failure #180
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AlexanderGrissik
force-pushed
the
fix_accept_failure
branch
from
2f20193
to
cfa56a0
Compare
iftahl
reviewed
Jul 10, 2024
| { | ||
| lock_tcp_con(); | ||
| child_conn->unlock_tcp_con(); | ||
|
|
There was a problem hiding this comment.
why don't we leave the child_conn locked through the whole function?
There was a problem hiding this comment.
Reminder: Remove accpted_conn loop.
There was a problem hiding this comment.
Removed unneeded m_accepted_conn loop after setting m_parent=nullptr before putting the conenction into m_accepted_conn
There was a problem hiding this comment.
We do need to unlock child because we lock listen socket. It will work without, but it is not a good practice to have two locks acquired by the same thread.
|
bot:retest |
AlexanderGrissik
force-pushed
the
fix_accept_failure
branch
from
cfa56a0
to
34ac738
Compare
|
The case of FIN at SYN_RCVD state will be handled on timer. This is our current behavior until we decide to change that. RFC/Web do not state what exact behavior should implementation take in this case. |
|
bot:retest |
AlexanderGrissik
force-pushed
the
vNext
branch
from
cfce2e8
to
51c2340
Compare
When delegate mode is enabled, incoming sockets with failed handshake are not closed immediately because in this mode socket object will be immediately destroyed in the middle of socket processing. This leads to access after destroy in related flows. Instead, we keep the socket in this list and the close() will be invoked as part of timers handling. We reuse the socket_fd_list_node_offset var to build the list, since this var is used for epoll and so guaranteed to be unused for half open sockets as application does not receive the fd for such sockets. The handling of closing SYN_RCVD socket as part of handling FIN in sockinfo_tcp was removed, since we ignore FIN packets in SYN_RCVD state. RFC https://datatracker.ietf.org/doc/html/rfc9293 does not state precisely how to handle this case. Call Hierarchy that may lead to termination of failed incoming connection: handle_incoming_handshake_failure: err_lwip_cb: L3_level_tcp_input - In order RST received tcp_slowtmr - SYN_RCVD timeout tcp_abandon: tcp_listen_input: - SYN-ACK enqueue failure L3_level_tcp_input tcp_pcb_reuse: - SYN-ACK enqueue failure tcp_timewait_input: L3_level_tcp_input tcp_abort: - Unrelated to incoming handshake failure L3_level_tcp_input tcp_process: L3_level_tcp_input sockinfo_tcp::abort_connection() Signed-off-by: Alexander Grissik <[email protected]>
The member sockinfo_tcp::m_received_syn_num is unused Signed-off-by: Alexander Grissik <[email protected]>
Removing precached variable to decrease sockinfo_tcp and CPU cache utilization. Signed-off-by: Alexander Grissik <[email protected]>
AlexanderGrissik
force-pushed
the
fix_accept_failure
branch
from
34ac738
to
d860bf0
Compare
iftahl
approved these changes
Aug 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Seg fault fix for delegated timers and cleanups
What
See commit comments.
Why ?
Crash fix for Nginx Proxy
Change type
What kind of change does this PR introduce?
Check list