quinn-rs / quinn

Async-friendly QUIC implementation in Rust
Apache License 2.0
3.54k stars 360 forks source link

[fix] the unit test gso_truncation #1853

Closed ylht closed 1 month ago

ylht commented 1 month ago

I find the unit test gso_truncation in this commit https://github.com/quinn-rs/quinn/commit/d5fc528930c0761215fcad13a7797e9904ae4b6a 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 https://github.com/quinn-rs/quinn/commit/d5fc528930c0761215fcad13a7797e9904ae4b6a. 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);
        }
ylht commented 1 month ago

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