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

[fix] the unit test gso_truncation #1853

Merged
merged 2 commits into from
May 8, 2024
Merged

[fix] the unit test gso_truncation #1853

merged 2 commits into from
May 8, 2024

Conversation

ylht
Copy link
Contributor

@ylht ylht commented May 8, 2024

I find the unit test gso_truncation in this commit d5fc528 is wrong. In fact, it does not really pass.

Specifically, I run the command cargo test --color=always --lib tests::gso_truncation -- --show-output to observe its output log. The client cannot decrypt the second packet, i.e., server: quinn_proto::connection: failed to authenticate packet.

2024-05-08T12:56:36.427721Z TRACE server: quinn_proto::connection: got Data packet (1053 bytes) from [::1]:44433 using id 4a9a80b04ca84a27
2024-05-08T12:56:36.427744Z TRACE server:recv{space=Data pn=10}: quinn_proto::connection: got datagram frame len=1024
2024-05-08T12:56:36.427761Z TRACE server: quinn_proto::connection: got Data packet (781 bytes) from [::1]:44433 using id 4a9a80b04ca84a27
2024-05-08T12:56:36.427776Z TRACE server: quinn_proto::connection::packet_crypto: decryption failed with packet number 10883353
2024-05-08T12:56:36.427783Z DEBUG server: quinn_proto::connection: failed to authenticate packet
2024-05-08T12:56:36.427790Z TRACE server: quinn_proto::connection: got Data packet (797 bytes) from [::1]:44433 using id 4a9a80b04ca84a27
2024-05-08T12:56:36.427810Z TRACE server:recv{space=Data pn=12}: quinn_proto::connection: got datagram frame len=768

After I delete the following code in the commit d5fc528. The unit test gso_truncation exactly pass.

                        let packet_len_unpadded = cmp::max(builder.min_size, buf.len())
                            - datagram_start
                            + builder.tag_len;
                        if packet_len_unpadded + MAX_PADDING < segment_size {
                            trace!(
                                "GSO truncated by demand for {} padding bytes",
                                segment_size - packet_len_unpadded
                            );
                            break;
                        }

2024-05-08T12:59:08.856743Z TRACE server: quinn_proto::connection: got Data packet (1053 bytes) from [::1]:44433 using id 8c32e49c8d5f276a
2024-05-08T12:59:08.856789Z TRACE server:recv{space=Data pn=10}: quinn_proto::connection: got datagram frame len=1024
2024-05-08T12:59:08.856824Z TRACE server: quinn_proto::connection: got Data packet (1053 bytes) from [::1]:44433 using id 8c32e49c8d5f276a
2024-05-08T12:59:08.856867Z TRACE server:recv{space=Data pn=11}: quinn_proto::connection: got datagram frame len=768
2024-05-08T12:59:08.856990Z TRACE server: quinn_proto::connection: got Data packet (797 bytes) from [::1]:44433 using id 8c32e49c8d5f276a
2024-05-08T12:59:08.857034Z TRACE server:recv{space=Data pn=12}: quinn_proto::connection: got datagram frame len=768

Finally, I expose the deep reason. In the line 680 if let Some(mut builder) = builder_storage.take() {, builder_storage.take has been None. Then we cannot send the last packet in Line 926-935, which is shown as following. So I re-insert builder into builder_storage in the PR. Then we pass the unix test gso_truncation completely.

        if let Some(mut builder) = builder_storage {
            if pad_datagram {
                builder.pad_to(MIN_INITIAL_SIZE);
            }
            let last_packet_number = builder.exact_number;
            builder.finish_and_track(now, self, sent_frames, buf);
            self.path
                .congestion
                .on_sent(now, buf.len() as u64, last_packet_number);
        }

Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Awesome catch, thank you! I took the liberty of making a cosmetic tweak and adjusting the test to detect this failure mode properly.

@djc djc merged commit f0dff01 into quinn-rs:main May 8, 2024
8 checks passed
@ylht
Copy link
Contributor Author

ylht commented May 8, 2024

No problem, thank you for your continuous contributions. It’s an honor to be able to offer some help to your work.

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.

3 participants