fix: Replace qpack::Data with trait (mozilla#3013)

Co-authored-by: Max Inden <mail@max-inden.de>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Author: 3 people

neqo-common/src/codec.rs

@@ -211,7 +211,6 @@ impl<'b> PartialEq<Decoder<'b>> for Decoder<'_> {
 }
 
 /// Encoder is good for building data structures.
-#[derive(Clone, PartialEq, Eq)]
 pub struct Encoder<B = Vec<u8>> {
     buf: B,
     /// Tracks the starting position of the buffer when the [`Encoder`] is created.
@@ -220,6 +219,23 @@ pub struct Encoder<B = Vec<u8>> {
     start: usize,
 }
 
+impl Clone for Encoder {
+    fn clone(&self) -> Self {
+        Self {
+            buf: self.as_ref().to_vec(),
+            start: 0,
+        }
+    }
+}
+
+impl<B: Buffer> PartialEq for Encoder<B> {
+    fn eq(&self, other: &Self) -> bool {
+        self.as_ref() == other.as_ref()
+    }
+}
+
+impl<B: Buffer> Eq for Encoder<B> {}
+
 impl<B: Buffer> Encoder<B> {
     /// Get the length of the [`Encoder`].
     ///
@@ -409,6 +425,17 @@ impl Encoder<Vec<u8>> {
         Self::default()
     }
 
+    /// Skip the first `n` bytes from the encoder buffer without copying.
+    /// This advances the internal offset, making those bytes inaccessible.
+    ///
+    /// # Panics
+    ///
+    /// Panics if `n` is greater than the current length of the encoder.
+    pub fn skip(&mut self, n: usize) {
+        assert!(n <= self.len(), "Cannot skip beyond buffer length");
+        self.start += n;
+    }
+
     /// Static helper function for previewing the results of encoding without doing it.
     ///
     /// # Panics
@@ -504,8 +531,11 @@ impl From<&[u8]> for Encoder {
 }
 
 impl From<Encoder> for Vec<u8> {
-    fn from(buf: Encoder) -> Self {
-        buf.buf
+    fn from(mut enc: Encoder) -> Self {
+        if enc.start > 0 {
+            enc.buf.drain(..enc.start);
+        }
+        enc.buf
     }
 }
 
@@ -1195,6 +1225,26 @@ mod tests {
         assert_eq!(Buffer::position(&buf), 0);
     }
 
+    #[test]
+    fn encoder_skip() {
+        let mut enc = Encoder::from_hex("010203040506");
+
+        enc.skip(2);
+        assert_eq!(enc.len(), 4);
+        assert_eq!(enc.as_ref(), &[0x03, 0x04, 0x05, 0x06]);
+
+        enc.skip(4);
+        assert_eq!(enc.len(), 0);
+        assert!(enc.is_empty());
+    }
+
+    #[test]
+    #[should_panic(expected = "Cannot skip beyond buffer length")]
+    fn encoder_skip_too_much() {
+        let mut enc = Encoder::from_hex("0102");
+        enc.skip(3);
+    }
+
     /// [`Encoder::as_decoder`] should only expose the bytes actively encoded through this
     /// [`Encoder`], not all bytes of the underlying [`Buffer`].
     #[test]
@@ -1208,4 +1258,42 @@ mod tests {
         assert_eq!(decoder.as_ref(), &[5, 6, 7]);
         assert_eq!(buffer, &[1, 2, 3, 4, 5, 6, 7]);
     }
+
+    /// Converting an [`Encoder`] to [`Vec<u8>`] should respect the `start` offset.
+    #[test]
+    fn into_vec_respects_skip() {
+        let mut enc = Encoder::from_hex("010203040506");
+        enc.skip(2);
+        let v: Vec<u8> = enc.into();
+        assert_eq!(v, vec![0x03, 0x04, 0x05, 0x06]);
+    }
+
+    /// Converting an [`Encoder`] without skip should return the full buffer.
+    #[test]
+    fn into_vec_without_skip() {
+        let enc = Encoder::from_hex("010203");
+        let v: Vec<u8> = enc.into();
+        assert_eq!(v, vec![0x01, 0x02, 0x03]);
+    }
+
+    /// [`PartialEq`] should compare the logical view.
+    #[test]
+    fn partial_eq_respects_skip() {
+        let mut enc1 = Encoder::from_hex("010203040506");
+        enc1.skip(2);
+        let enc2 = Encoder::from_hex("03040506");
+        assert_eq!(enc1, enc2);
+    }
+
+    /// [`Clone`] should not clone skipped bytes.
+    #[test]
+    fn clone_respects_skip() {
+        let mut enc = Encoder::from_hex("010203040506");
+        enc.skip(2);
+        let cloned = enc.clone();
+        assert_eq!(cloned.as_ref(), &[0x03, 0x04, 0x05, 0x06]);
+        assert_eq!(cloned.len(), 4);
+        let v: Vec<u8> = cloned.into();
+        assert_eq!(v, vec![0x03, 0x04, 0x05, 0x06]);
+    }
 }

neqo-qpack/src/decoder.rs

@@ -6,14 +6,13 @@
 
 use std::fmt::{self, Display, Formatter};
 
-use neqo_common::{qdebug, Header};
+use neqo_common::{qdebug, Encoder, Header};
 use neqo_transport::{Connection, StreamId};
 
 use crate::{
     decoder_instructions::DecoderInstruction,
     encoder_instructions::{DecodedEncoderInstruction, EncoderInstructionReader},
     header_block::{HeaderDecoder, HeaderDecoderResult},
-    qpack_send_buf::Data,
     reader::{ReadByte, Reader, ReceiverConnWrapper},
     stats::Stats,
     table::HeaderTable,
@@ -28,7 +27,7 @@ pub struct Decoder {
     table: HeaderTable,
     acked_inserts: u64,
     max_entries: u64,
-    send_buf: Data,
+    send_buf: Encoder,
     local_stream_id: Option<StreamId>,
     max_table_size: u64,
     max_blocked_streams: usize,
@@ -43,7 +42,7 @@ impl Decoder {
     #[must_use]
     pub fn new(qpack_settings: &Settings) -> Self {
         qdebug!("Decoder: creating a new qpack decoder");
-        let mut send_buf = Data::default();
+        let mut send_buf = Encoder::default();
         send_buf.encode_varint(QPACK_UNI_STREAM_TYPE_DECODER);
         let max_blocked_streams = usize::from(qpack_settings.max_blocked_streams);
         Self {
@@ -179,11 +178,11 @@ impl Decoder {
             let r = conn
                 .stream_send(
                     self.local_stream_id.ok_or(Error::Internal)?,
-                    &self.send_buf[..],
+                    self.send_buf.as_ref(),
                 )
                 .map_err(|_| Error::DecoderStream)?;
             qdebug!("[{self}] {r} bytes sent");
-            self.send_buf.read(r);
+            self.send_buf.skip(r);
         }
         Ok(())
     }

neqo-qpack/src/decoder_instructions.rs

@@ -14,7 +14,7 @@ use neqo_transport::StreamId;
 
 use crate::{
     prefix::{DECODER_HEADER_ACK, DECODER_INSERT_COUNT_INCREMENT, DECODER_STREAM_CANCELLATION},
-    qpack_send_buf::Data,
+    qpack_send_buf::Encoder,
     reader::{IntReader, ReadByte},
     Res,
 };
@@ -44,7 +44,7 @@ impl DecoderInstruction {
         }
     }
 
-    pub(crate) fn marshal(&self, enc: &mut Data) {
+    pub(crate) fn marshal<T: Encoder>(&self, enc: &mut T) {
         match self {
             Self::InsertCountIncrement { increment } => {
                 enc.encode_prefixed_encoded_int(DECODER_INSERT_COUNT_INCREMENT, *increment);
@@ -144,16 +144,17 @@ impl DecoderInstructionReader {
 #[cfg_attr(coverage_nightly, coverage(off))]
 mod test {
 
+    use neqo_common::Encoder;
     use neqo_transport::StreamId;
 
-    use super::{Data, DecoderInstruction, DecoderInstructionReader};
+    use super::{DecoderInstruction, DecoderInstructionReader};
     use crate::{reader::test_receiver::TestReceiver, Error};
 
     fn test_encoding_decoding(instruction: DecoderInstruction) {
-        let mut buf = Data::default();
+        let mut buf = Encoder::default();
         instruction.marshal(&mut buf);
         let mut test_receiver: TestReceiver = TestReceiver::default();
-        test_receiver.write(&buf);
+        test_receiver.write(buf.as_ref());
         let mut decoder = DecoderInstructionReader::new();
         assert_eq!(
             decoder.read_instructions(&mut test_receiver).unwrap(),
@@ -182,18 +183,18 @@ mod test {
     }
 
     fn test_encoding_decoding_slow_reader(instruction: DecoderInstruction) {
-        let mut buf = Data::default();
+        let mut buf = Encoder::default();
         instruction.marshal(&mut buf);
         let mut test_receiver: TestReceiver = TestReceiver::default();
         let mut decoder = DecoderInstructionReader::new();
         for i in 0..buf.len() - 1 {
-            test_receiver.write(&buf[i..=i]);
+            test_receiver.write(&buf.as_ref()[i..=i]);
             assert_eq!(
                 decoder.read_instructions(&mut test_receiver),
                 Err(Error::NeedMoreData)
             );
         }
-        test_receiver.write(&buf[buf.len() - 1..buf.len()]);
+        test_receiver.write(&buf.as_ref()[buf.len() - 1..buf.len()]);
         assert_eq!(
             decoder.read_instructions(&mut test_receiver).unwrap(),
             instruction

neqo-qpack/src/encoder.rs

@@ -20,7 +20,6 @@ use crate::{
     encoder_instructions::EncoderInstruction,
     header_block::HeaderEncoder,
     qlog,
-    qpack_send_buf::Data,
     reader::ReceiverConnWrapper,
     stats::Stats,
     table::{HeaderTable, LookupResult, ADDITIONAL_TABLE_ENTRY_SIZE},
@@ -281,14 +280,14 @@ impl Encoder {
             return Err(Error::DynamicTableFull);
         }
 
-        let mut buf = Data::default();
+        let mut buf = neqo_common::Encoder::default();
         EncoderInstruction::InsertWithNameLiteral { name, value }
             .marshal(&mut buf, self.use_huffman);
 
         let stream_id = self.local_stream.stream_id().ok_or(Error::Internal)?;
 
         let sent = conn
-            .stream_send_atomic(stream_id, &buf)
+            .stream_send_atomic(stream_id, buf.as_ref())
             .map_err(|e| map_stream_send_atomic_error(&e))?;
         if !sent {
             return Err(Error::EncoderStreamBlocked);
@@ -321,9 +320,9 @@ impl Encoder {
             if cap < self.table.capacity() && !self.table.can_evict_to(cap) {
                 return Err(Error::DynamicTableFull);
             }
-            let mut buf = Data::default();
+            let mut buf = neqo_common::Encoder::default();
             EncoderInstruction::Capacity { value: cap }.marshal(&mut buf, self.use_huffman);
-            if !conn.stream_send_atomic(stream_id, &buf)? {
+            if !conn.stream_send_atomic(stream_id, buf.as_ref())? {
                 return Err(Error::EncoderStreamBlocked);
             }
             if self.table.set_capacity(cap).is_err() {
@@ -351,9 +350,9 @@ impl Encoder {
                 Ok(())
             }
             LocalStreamState::Uninitialized(stream_id) => {
-                let mut buf = Data::default();
+                let mut buf = neqo_common::Encoder::default();
                 buf.encode_varint(QPACK_UNI_STREAM_TYPE_ENCODER);
-                if !conn.stream_send_atomic(stream_id, &buf[..])? {
+                if !conn.stream_send_atomic(stream_id, buf.as_ref())? {
                     return Err(Error::EncoderStreamBlocked);
                 }
                 self.local_stream = LocalStreamState::Initialized(stream_id);
@@ -596,7 +595,7 @@ mod tests {
             let buf = self
                 .encoder
                 .encode_header_block(&mut self.conn, headers, stream_id);
-            assert_eq!(&buf[..], expected_encoding);
+            assert_eq!(buf.as_ref(), expected_encoding);
             self.send_instructions(inst);
         }
 
@@ -822,7 +821,7 @@ mod tests {
             let buf = encoder
                 .encoder
                 .encode_header_block(&mut encoder.conn, &t.headers, STREAM_1);
-            assert_eq!(&buf[..], t.header_block);
+            assert_eq!(buf.as_ref(), t.header_block);
             encoder.send_instructions(t.encoder_inst);
         }
     }
@@ -896,7 +895,7 @@ mod tests {
             let buf = encoder
                 .encoder
                 .encode_header_block(&mut encoder.conn, &t.headers, STREAM_1);
-            assert_eq!(&buf[..], t.header_block);
+            assert_eq!(buf.as_ref(), t.header_block);
             encoder.send_instructions(t.encoder_inst);
         }
     }
@@ -968,7 +967,7 @@ mod tests {
             &[Header::new("content-length", "1234")],
             STREAM_1,
         );
-        assert_eq!(&buf[..], ENCODE_INDEXED_REF_DYNAMIC);
+        assert_eq!(buf.as_ref(), ENCODE_INDEXED_REF_DYNAMIC);
         encoder.send_instructions(&[]);
 
         // insert "content-length: 12345 which will fail because the entry in the table cannot be

neqo-qpack/src/encoder_instructions.rs

@@ -16,7 +16,7 @@ use crate::{
         ENCODER_CAPACITY, ENCODER_DUPLICATE, ENCODER_INSERT_WITH_NAME_LITERAL,
         ENCODER_INSERT_WITH_NAME_REF_DYNAMIC, ENCODER_INSERT_WITH_NAME_REF_STATIC, NO_PREFIX,
     },
-    qpack_send_buf::Data,
+    qpack_send_buf::Encoder,
     reader::{IntReader, LiteralReader, ReadByte, Reader},
     Res,
 };
@@ -52,7 +52,7 @@ pub enum EncoderInstruction<'a> {
 }
 
 impl EncoderInstruction<'_> {
-    pub(crate) fn marshal(&self, enc: &mut Data, use_huffman: bool) {
+    pub(crate) fn marshal<T: Encoder>(&self, enc: &mut T, use_huffman: bool) {
         match self {
             Self::Capacity { value } => {
                 enc.encode_prefixed_encoded_int(ENCODER_CAPACITY, *value);
@@ -297,14 +297,14 @@ impl EncoderInstructionReader {
 #[cfg_attr(coverage_nightly, coverage(off))]
 mod test {
 
-    use super::{Data, EncoderInstruction, EncoderInstructionReader};
+    use super::{EncoderInstruction, EncoderInstructionReader};
     use crate::{reader::test_receiver::TestReceiver, Error};
 
     fn test_encoding_decoding(instruction: &EncoderInstruction, use_huffman: bool) {
-        let mut buf = Data::default();
+        let mut buf = neqo_common::Encoder::default();
         instruction.marshal(&mut buf, use_huffman);
         let mut test_receiver: TestReceiver = TestReceiver::default();
-        test_receiver.write(&buf);
+        test_receiver.write(buf.as_ref());
         let mut reader = EncoderInstructionReader::new();
         assert_eq!(
             reader.read_instructions(&mut test_receiver).unwrap(),
@@ -395,18 +395,18 @@ mod test {
     }
 
     fn test_encoding_decoding_slow_reader(instruction: &EncoderInstruction, use_huffman: bool) {
-        let mut buf = Data::default();
+        let mut buf = neqo_common::Encoder::default();
         instruction.marshal(&mut buf, use_huffman);
         let mut test_receiver: TestReceiver = TestReceiver::default();
         let mut decoder = EncoderInstructionReader::new();
         for i in 0..buf.len() - 1 {
-            test_receiver.write(&buf[i..=i]);
+            test_receiver.write(&buf.as_ref()[i..=i]);
             assert_eq!(
                 decoder.read_instructions(&mut test_receiver),
                 Err(Error::NeedMoreData)
             );
         }
-        test_receiver.write(&buf[buf.len() - 1..buf.len()]);
+        test_receiver.write(&buf.as_ref()[buf.len() - 1..buf.len()]);
         assert_eq!(
             decoder.read_instructions(&mut test_receiver).unwrap(),
             instruction.into()