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

use orig_dst_cid to add into index to avoid corruption #1980

Closed

Conversation

lijunwangs
Copy link
Contributor

Problem:

When the initial packet is a stateless retry packet, we use the orig_dst_cid to create the quinn_proto::Incoming. However we added the connection into the index using the header.dst_cid. These two connection CID can be different. When the connection is later removed from the index, we use the one populated in the Incoming. This will fail to properly remove it. This causes discrepancy of the Quinn_proto::Endpoint::index and Quinn_proto::Endpoint::incoming_buffers and cause crashes when the incoming buffer is accessed. A scenario

  1. A stateless connect retry initial packet is received, we add (header.dst_cid, index) into the connection index and incoming buffers.
  2. The connection is ignored by the application which triggers clean_up_incoming and cleanup, This removes the corresponding index from the incoming_buffers but not the Connection from the index (which was added using header.dst_cid).
  3. Client sends the connection retry packet again with the same header.dst_cid, in this case we found the connection from Endpoint::index, and obtained RouteDatagramTo::Incoming, with the index value in it, when accessing
    let incoming_buffer = &mut self.incoming_buffers[incoming_idx];
    it will blows up due to wrong index as it was removed in step 2.

Changes:
in Quinn_proto::Endpoint::handle_first_packet use the orig_dst_cid variable to the index to keep consistent with the Incoming created.

@lijunwangs lijunwangs marked this pull request as draft September 3, 2024 08:11
@lijunwangs
Copy link
Contributor Author

This seems to have been addressed by e01609c.

@lijunwangs lijunwangs closed this Sep 3, 2024
@Ralith
Copy link
Collaborator

Ralith commented Sep 4, 2024

Thanks for the diagnosis, all the same!

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