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

prevent sending data on a uTP socket after closing it #7162

Merged
merged 1 commit into from
Oct 21, 2022

Conversation

arvidn
Copy link
Owner

@arvidn arvidn commented Oct 20, 2022

No description provided.

@arvidn arvidn force-pushed the utp-socket-write-after-close branch from ffa0fad to b950355 Compare October 20, 2022 13:00
@thrnz
Copy link
Contributor

thrnz commented Oct 21, 2022

This seems to do the job. I've not yet been able to trigger it happening again using this PR.

@arvidn arvidn merged commit 40a2f6f into RC_2_0 Oct 21, 2022
@arvidn arvidn deleted the utp-socket-write-after-close branch October 21, 2022 01:10
@@ -1394,6 +1405,10 @@ bool utp_socket_impl::send_pkt(int const flags)
+ (sack ? sack + 2 : 0)
+ (close_reason ? 6 : 0);

// once we're in fin-sent mode, the write buffer should not be re-filled
// although, we may re-send packets, but those live in m_outbuf
TORRENT_ASSERT(state() != state_t::fin_sent || m_write_buffer_size == 0);
Copy link
Owner Author

Choose a reason for hiding this comment

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

this assert fails. The fundamental issue is that the utp implementation doesn't handle one-sided closing of the connection.

When receiving a FIN, we correctly note the sequence number. We reject more data past that sequence number but allow re-sends of previous packets. However, currently we also respond with a FIN immediately (without the consent of the upper layer). It's not great.

Ideally, an incoming FIN should be acked immediately, and only close the incoming stream, and produce an EOF when the upper layer reads up against it. The outgoing FIN should only be sent when the upper layer closes the socket. Which means the outgoing sequence number needs to be recorded when closing the socket, and then assert we don't try to send any data past that.

@arvidn arvidn mentioned this pull request Jan 2, 2023
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.

2 participants