openpgp: Make Subpacket own the data.

  - The subpacket areas now have a vector of subpackets.  Change some
    accessors here and there to accommodate this.

  - This requires bit-perfect roundtripping of subpackets so that
    signatures are not invalidated.

  - First step towards fixing #200.

10 files changed, 369 insertions, 350 deletions

diff --git a/openpgp/examples/statistics.rs b/openpgp/examples/statistics.rs
index 7f4f00c2..0b453467 100644
--- a/openpgp/examples/statistics.rs
+++ b/openpgp/examples/statistics.rs
@@ -15,6 +15,7 @@ use crate::openpgp::types::*;
 use crate::openpgp::packet::{user_attribute, header::BodyLength, Tag};
 use crate::openpgp::packet::signature::subpacket::SubpacketTag;
 use crate::openpgp::parse::{Parse, PacketParserResult, PacketParser};
+use crate::openpgp::serialize::SerializeInto;
 
 fn main() {
     let args: Vec<String> = env::args().collect();
@@ -140,7 +141,7 @@ fn main() {
                     cert.sigs[u8::from(sig.typ()) as usize] += 1;
                     let mut signature = PerSignature::min();
 
-                    for (_offset, len, sub) in sig.hashed_area().iter()
+                    for sub in sig.hashed_area().iter()
                         .chain(sig.unhashed_area().iter())
                     {
                         use crate::openpgp::packet::signature::subpacket::*;
@@ -152,6 +153,7 @@ fn main() {
                             sigs_subpacket_tags_unknown
                                 [u8::from(sub.tag()) as usize] += 1;
                         } else {
+                            let len = sub.serialized_len();
                             sigs_subpacket_tags_size_bytes[i] += len;
                             sigs_subpacket_tags_size_count[i] += 1;
                             let len = len as u32;
diff --git a/openpgp/src/cert/mod.rs b/openpgp/src/cert/mod.rs
index cb0de264..66f9fef4 100644
--- a/openpgp/src/cert/mod.rs
+++ b/openpgp/src/cert/mod.rs
@@ -2627,9 +2627,9 @@ mod test {
             .build(&mut keypair, &cert, None)
             .unwrap();
         assert_eq!(sig.typ(), SignatureType::KeyRevocation);
-        assert_eq!(sig.issuer(), Some(cert.primary().keyid()));
+        assert_eq!(sig.issuer(), Some(&cert.primary().keyid()));
         assert_eq!(sig.issuer_fingerprint(),
-                   Some(cert.primary().fingerprint()));
+                   Some(&cert.primary().fingerprint()));
 
         let cert = cert.merge_packets(vec![sig.into()]).unwrap();
         assert_match!(RevocationStatus::Revoked(_) = cert.revoked(None));
@@ -2650,9 +2650,9 @@ mod test {
             .unwrap();
 
         assert_eq!(sig.typ(), SignatureType::KeyRevocation);
-        assert_eq!(sig.issuer(), Some(other.primary().keyid()));
+        assert_eq!(sig.issuer(), Some(&other.primary().keyid()));
         assert_eq!(sig.issuer_fingerprint(),
-                   Some(other.primary().fingerprint()));
+                   Some(&other.primary().fingerprint()));
     }
 
     #[test]
diff --git a/openpgp/src/crypto/hash.rs b/openpgp/src/crypto/hash.rs
index 539970f1..05bff4c6 100644
--- a/openpgp/src/crypto/hash.rs
+++ b/openpgp/src/crypto/hash.rs
@@ -296,6 +296,12 @@ impl Hash for Signature4 {
 impl Hash for signature::Builder {
     /// Adds the `Signature` to the provided hash context.
     fn hash(&self, hash: &mut Context) {
+        use crate::serialize::SerializeInto;
+        // XXX: Annoyingly, we have no proper way of handling errors
+        // here.
+        let hashed_area = self.hashed_area().to_vec()
+            .unwrap_or_else(|_| Vec::new());
+
         // A version 4 signature packet is laid out as follows:
         //
         //   version - 1 byte                    \
@@ -315,13 +321,12 @@ impl Hash for signature::Builder {
         header[3] = self.hash_algo().into();
 
         // The length of the hashed area, as a 16-bit endian number.
-        let len = self.hashed_area().data.len();
+        let len = hashed_area.len();
         header[4] = (len >> 8) as u8;
         header[5] = len as u8;
 
         hash.update(&header[..]);
-
-        hash.update(&self.hashed_area().data[..]);
+        hash.update(&hashed_area);
 
         // A version 4 signature trailer is:
         //
@@ -340,7 +345,7 @@ impl Hash for signature::Builder {
         trailer[1] = 0xff;
         // The signature packet's length, not including the previous
         // two bytes and the length.
-        let len = header.len() + self.hashed_area().data.len();
+        let len = header.len() + hashed_area.len();
         trailer[2] = (len >> 24) as u8;
         trailer[3] = (len >> 16) as u8;
         trailer[4] = (len >> 8) as u8;
diff --git a/openpgp/src/packet/one_pass_sig.rs b/openpgp/src/packet/one_pass_sig.rs
index c3f017ed..b2430a82 100644
--- a/openpgp/src/packet/one_pass_sig.rs
+++ b/openpgp/src/packet/one_pass_sig.rs
@@ -156,7 +156,7 @@ impl<'a> std::convert::TryFrom<&'a Signature> for OnePassSig3 {
 
     fn try_from(s: &'a Signature) -> Result<Self> {
         let issuer = match s.issuer() {
-            Some(i) => i,
+            Some(i) => i.clone(),
             None =>
                 return Err(Error::InvalidArgument(
                     "Signature has no issuer".into()).into()),
diff --git a/openpgp/src/packet/signature/mod.rs b/openpgp/src/packet/signature/mod.rs
index 3dc7f9e1..e508ecd6 100644
--- a/openpgp/src/packet/signature/mod.rs
+++ b/openpgp/src/packet/signature/mod.rs
@@ -523,7 +523,7 @@ impl Signature4 {
 
         self.hashed_area().iter()
             .chain(self.unhashed_area().iter())
-            .filter_map(|(_, _, subpacket)| {
+            .filter_map(|subpacket| {
                 match subpacket.value() {
                     SubpacketValue::Issuer(i) =>
                         Some(crate::KeyHandle::KeyID(i.clone())),
@@ -563,12 +563,13 @@ impl Signature4 {
                 // Fall back to the Issuer, which we will also get
                 // from the unhashed area if necessary.
                 area.add(Subpacket::new(
-                    SubpacketValue::Issuer(issuer), false).unwrap()).unwrap();
+                    SubpacketValue::Issuer(issuer.clone()), false).unwrap())
+                    .unwrap();
             }
 
             // Second, re-add the EmbeddedSignature, if present.
             if let Some(embedded_sig) =
-                self.unhashed_area().iter().find_map(|(_, _, v)| {
+                self.unhashed_area().iter().find_map(|v| {
                     if v.tag() == SubpacketTag::EmbeddedSignature {
                         Some(v)
                     } else {
@@ -576,7 +577,7 @@ impl Signature4 {
                     }
                 })
             {
-                area.add(embedded_sig).unwrap();
+                area.add(embedded_sig.clone()).unwrap();
             }
         }
         sig
@@ -1510,10 +1511,10 @@ mod test {
         let sig = builder.sign_hash(&mut pair,
                                     hash.clone()).unwrap().normalize();
         assert_eq!(sig.unhashed_area().iter().count(), 2);
-        assert_eq!(sig.unhashed_area().iter().nth(0).unwrap().2,
+        assert_eq!(*sig.unhashed_area().iter().nth(0).unwrap(),
                    Subpacket::new(SubpacketValue::Issuer(keyid.clone()),
                                   false).unwrap());
-        assert_eq!(sig.unhashed_area().iter().nth(1).unwrap().2.tag(),
+        assert_eq!(sig.unhashed_area().iter().nth(1).unwrap().tag(),
                    SubpacketTag::EmbeddedSignature);
 
         // Now, make sure that an Issuer subpacket is synthesized from
@@ -1523,7 +1524,7 @@ mod test {
             .sign_hash(&mut pair,
                        hash.clone()).unwrap().normalize();
         assert_eq!(sig.unhashed_area().iter().count(), 1);
-        assert_eq!(sig.unhashed_area().iter().nth(0).unwrap().2,
+        assert_eq!(*sig.unhashed_area().iter().nth(0).unwrap(),
                    Subpacket::new(SubpacketValue::Issuer(keyid.clone()),
                                   false).unwrap());
     }
diff --git a/openpgp/src/packet/signature/subpacket.rs b/openpgp/src/packet/signature/subpacket.rs
index 96bb3edc..47b8b8f3 100644
--- a/openpgp/src/packet/signature/subpacket.rs
+++ b/openpgp/src/packet/signature/subpacket.rs
@@ -58,7 +58,6 @@ use std::cell::RefCell;
 use std::collections::HashMap;
 use std::convert::TryInto;
 use std::hash::{Hash, Hasher};
-use std::iter::FromIterator;
 use std::sync::Mutex;
 use std::ops::{Deref, DerefMut};
 use std::fmt;
@@ -323,6 +322,7 @@ mod tests {
 //
 // The value is uninterpreted.
 struct SubpacketRaw<'a> {
+    length: SubpacketLength,
     pub critical: bool,
     pub tag: SubpacketTag,
     pub value: &'a [u8],
@@ -337,6 +337,7 @@ impl<'a> fmt::Debug for SubpacketRaw<'a> {
         };
 
         f.debug_struct("SubpacketRaw")
+            .field("length", &self.length)
             .field("critical", &self.critical)
             .field("tag", &self.tag)
             .field(&format!("value ({} bytes)", self.value.len())[..],
@@ -347,29 +348,32 @@ impl<'a> fmt::Debug for SubpacketRaw<'a> {
 
 /// Subpacket area.
 pub struct SubpacketArea {
-    /// Raw, unparsed subpacket data.
-    pub data: Vec<u8>,
+    /// The subpackets.
+    packets: Vec<Subpacket>,
 
     // The subpacket area, but parsed so that the map is indexed by
     // the subpacket tag, and the value corresponds to the *last*
     // occurrence of that subpacket in the subpacket area.
     //
-    // Since self-referential structs are a no-no, we use (start, len)
+    // Since self-referential structs are a no-no, we use an index
     // to reference the content in the area.
     //
     // This is an option, because we parse the subpacket area lazily.
-    parsed: Mutex<RefCell<Option<HashMap<SubpacketTag, (bool, u16, u16)>>>>,
+    parsed: Mutex<RefCell<Option<HashMap<SubpacketTag, usize>>>>,
 }
 
 impl Clone for SubpacketArea {
     fn clone(&self) -> Self {
-        Self::new(self.data.clone())
+        Self {
+            packets: self.packets.clone(),
+            parsed: Mutex::new(RefCell::new(None)),
+        }
     }
 }
 
 impl PartialEq for SubpacketArea {
     fn eq(&self, other: &SubpacketArea) -> bool {
-        self.data == other.data
+        self.packets == other.packets
     }
 }
 impl Eq for SubpacketArea {}
@@ -378,7 +382,7 @@ impl Hash for SubpacketArea {
     fn hash<H: Hasher>(&self, state: &mut H) {
         // We hash only the data, the cache is a hashmap and does not
         // implement hash.
-        self.data.hash(state);
+        self.packets.hash(state);
     }
 }
 
@@ -393,11 +397,11 @@ impl<'a> Iterator for SubpacketAreaIterRaw<'a> {
     type Item = (usize, usize, SubpacketRaw<'a>);
 
     fn next(&mut self) -> Option<Self::Item> {
-        let len = SubpacketLength::parse(&mut self.reader);
-        if len.is_err() {
-            return None;
-        }
-        let len = len.unwrap().len as usize;
+        let length = match SubpacketLength::parse(&mut self.reader) {
+            Err(_) => return None,
+            Ok(v) => v,
+        };
+        let len = length.len as usize;
 
         if self.reader.data(len).unwrap().len() < len {
             // Subpacket extends beyond the end of the hashed
@@ -410,6 +414,7 @@ impl<'a> Iterator for SubpacketAreaIterRaw<'a> {
         if len == 0 {
             // Hmm, a zero length packet.  In that case, there is
             // no header.
+            // XXX: Return an error.  See #200.
             return self.next();
         }
 
@@ -433,6 +438,7 @@ impl<'a> Iterator for SubpacketAreaIterRaw<'a> {
 
         Some((start, len,
               SubpacketRaw {
+                  length,
                   critical: critical,
                   tag: tag.into(),
                   value: &self.data[start..start + len],
@@ -440,80 +446,54 @@ impl<'a> Iterator for SubpacketAreaIterRaw<'a> {
     }
 }
 
-impl SubpacketArea {
-    fn iter_raw(&self) -> SubpacketAreaIterRaw {
+impl<'a> SubpacketAreaIterRaw<'a> {
+    fn new(data: &'a [u8]) -> SubpacketAreaIterRaw<'a> {
         SubpacketAreaIterRaw {
-            reader: buffered_reader::Memory::new(&self.data[..]),
-            data: &self.data[..],
+            reader: buffered_reader::Memory::new(data),
+            data,
         }
     }
 }
 
 impl fmt::Debug for SubpacketArea {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        f.debug_list().entries(
-            self.iter_raw().map(|(_start, _len, sb)| {
-                Subpacket::from(sb)
-            }))
+        f.debug_list()
+            .entries(self.iter())
             .finish()
     }
 }
 
-/// Iterates over SubpacketAreas yielding subpackets.
-pub struct Iter<'a> {
-    inner: SubpacketAreaIterRaw<'a>,
-}
-
-impl<'a> Iterator for Iter<'a> {
-    // Start, length, packet.
-    type Item = (usize, usize, Subpacket<'a>);
-
-    fn next(&mut self) -> Option<Self::Item> {
-        self.inner.next()
-            .map(|(start, len, raw)| (start, len, raw.into()))
-    }
-}
-
 impl<'a> IntoIterator for &'a SubpacketArea {
-    type Item = (usize, usize, Subpacket<'a>);
-    type IntoIter = Iter<'a>;
+    type Item = &'a Subpacket;
+    type IntoIter = std::slice::Iter<'a, Subpacket>;
 
     fn into_iter(self) -> Self::IntoIter {
-        self.iter()
-    }
-}
-
-impl<'a> FromIterator<(usize, usize, Subpacket<'a>)> for SubpacketArea {
-    fn from_iter<I>(iter: I) -> Self
-        where I: IntoIterator<Item=(usize, usize, Subpacket<'a>)>
-    {
-        use crate::serialize::Serialize;
-        let mut data = Vec::new();
-        iter.into_iter().for_each(|(_, _, s)| s.serialize(&mut data).unwrap());
-        Self::new(data)
+        self.packets.iter()
     }
 }
 
 impl SubpacketArea {
     /// Returns a new subpacket area based on `data`.
-    pub fn new(data: Vec<u8>) -> SubpacketArea {
-        SubpacketArea { data: data, parsed: Mutex::new(RefCell::new(None)) }
+    pub fn new(data: &[u8]) -> SubpacketArea {
+        SubpacketArea {
+            packets: SubpacketAreaIterRaw::new(data)
+                .map(|(_, _, raw)| raw.into()).collect(),
+            parsed: Mutex::new(RefCell::new(None)),
+        }
     }
 
     /// Returns a empty subpacket area.
     pub fn empty() -> SubpacketArea {
-        SubpacketArea::new(Vec::new())
+        SubpacketArea::new(&[])
     }
-}
 
-impl SubpacketArea {
     // Initialize `Signature::hashed_area_parsed` from
     // `Signature::hashed_area`, if necessary.
     fn cache_init(&self) {
         if self.parsed.lock().unwrap().borrow().is_none() {
             let mut hash = HashMap::new();
-            for (start, len, sb) in self.iter_raw() {
-                hash.insert(sb.tag, (sb.critical, start as u16, len as u16));
+            for (i, sp) in self.packets.iter().enumerate() {
+                hash.insert(sp.tag, i);
             }
 
             *self.parsed.lock().unwrap().borrow_mut() = Some(hash);
@@ -526,22 +506,16 @@ impl SubpacketArea {
     }
 
     /// Iterates over the subpackets.
-    pub fn iter<'a>(&'a self) -> Iter<'a> {
-        Iter { inner: self.iter_raw(), }
+    pub fn iter<'a>(&'a self) -> impl Iterator<Item = &'a Subpacket> {
+        self.packets.iter()
     }
 
     /// Returns the last subpacket, if any, with the specified tag.
-    pub fn lookup(&self, tag: SubpacketTag) -> Option<Subpacket> {
+    pub fn lookup(&self, tag: SubpacketTag) -> Option<&Subpacket> {
         self.cache_init();
 
         match self.parsed.lock().unwrap().borrow().as_ref().unwrap().get(&tag) {
-            Some(&(critical, start, len)) =>
-                return Some(SubpacketRaw {
-                    critical: critical,
-                    tag: tag,
-                    value: &self.data[
-                        start as usize..start as usize + len as usize]
-                }.into()),
+            Some(&n) => Some(&self.packets[n]),
             None => None,
         }
     }
@@ -553,15 +527,18 @@ impl SubpacketArea {
     /// Returns `Error::MalformedPacket` if adding the packet makes
     /// the subpacket area exceed the size limit.
     pub fn add(&mut self, packet: Subpacket) -> Result<()> {
-        use crate::serialize::Serialize;
+        use crate::serialize::SerializeInto;
 
-        if self.data.len() + packet.len() > ::std::u16::MAX as usize {
+        if self.serialized_len() + packet.serialized_len()
+            > ::std::u16::MAX as usize
+        {
             return Err(Error::MalformedPacket(
                 "Subpacket area exceeds maximum size".into()).into());
         }
 
         self.cache_invalidate();
-        packet.serialize(&mut self.data)
+        self.packets.push(packet);
+        Ok(())
     }
 
     /// Adds the given subpacket, replacing all other subpackets with
@@ -572,12 +549,17 @@ impl SubpacketArea {
     /// Returns `Error::MalformedPacket` if adding the packet makes
     /// the subpacket area exceed the size limit.
     pub fn replace(&mut self, packet: Subpacket) -> Result<()> {
-        let old = self.remove_all(packet.tag);
-        if let Err(e) = self.add(packet) {
-            // Restore old state.
-            self.data = old;
-            return Err(e);
+        use crate::serialize::SerializeInto;
+        if self.iter().filter_map(|sp| if sp.tag() != packet.tag() {
+            Some(sp.serialized_len())
+        } else {
+            None
+        }).sum::<usize>() > std::u16::MAX as usize {
+            return Err(Error::MalformedPacket(
+                "Subpacket area exceeds maximum size".into()).into());
         }
+        self.remove_all(packet.tag());
+        self.packets.push(packet);
         Ok(())
     }
 
@@ -585,54 +567,36 @@ impl SubpacketArea {
     ///
     /// Returns the old subpacket area, so that it can be restored if
     /// necessary.
-    pub fn remove_all(&mut self, tag: SubpacketTag) -> Vec<u8> {
-        let mut new = Vec::new();
-
-        // Copy all but the matching subpackets.
-        for (_, _, raw) in self.iter_raw() {
-            if raw.tag == tag {
-                // Drop.
-                continue;
-            }
-
-            let l = SubpacketLength::from(1 + raw.value.len() as u32);
-            let tag = u8::from(raw.tag)
-                | if raw.critical { 1 << 7 } else { 0 };
-
-            l.serialize(&mut new).unwrap();
-            new.push(tag);
-            new.extend_from_slice(raw.value);
-        }
-
+    pub fn remove_all(&mut self, tag: SubpacketTag) {
         self.cache_invalidate();
-        ::std::mem::replace(&mut self.data, new)
+        self.packets.retain(|sp| sp.tag() != tag);
     }
 
     /// Removes all subpackets.
     pub fn clear(&mut self) {
         self.cache_invalidate();
-        self.data.clear();
+        self.packets.clear();
     }
 
 }
 
 /// Payload of a NotationData subpacket.
-#[derive(Debug, PartialEq, Clone)]
-pub struct NotationData<'a> {
+#[derive(Clone, Debug, PartialEq, Eq, Hash)]
+pub struct NotationData {
     flags: NotationDataFlags,
-    name: &'a [u8],
-    value: &'a [u8],
+    name: Vec<u8>,
+    value: Vec<u8>,
 }
 
-impl<'a> NotationData<'a> {
+impl NotationData {
     /// Creates a new Notation Data subpacket payload.
-    pub fn new<F>(name: &'a str, value: &'a [u8], flags: F) -> Self
+    pub fn new<F>(name: &str, value: &[u8], flags: F) -> Self
         where F: Into<Option<NotationDataFlags>>
     {
         Self {
             flags: flags.into().unwrap_or_default(),
-            name: name.as_bytes(),
-            value,
+            name: name.into(),
+            value: value.into(),
         }
     }
 
@@ -642,18 +606,18 @@ impl<'a> NotationData<'a> {
     }
 
     /// Returns the name.
-    pub fn name(&self) -> &'a [u8] {
-        self.name
+    pub fn name(&self) -> &[u8] {
+        &self.name
     }
 
     /// Returns the value.
-    pub fn value(&self) -> &'a [u8] {
-        self.value
+    pub fn value(&self) -> &[u8] {
+        &self.value
     }
 }
 
 /// Flags for the Notation Data subpacket.
-#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
 pub struct NotationDataFlags(u32);
 
 impl Default for NotationDataFlags {
@@ -693,12 +657,12 @@ impl NotationDataFlags {
 ///
 /// The value is well structured.  See `SubpacketTag` for a
 /// description of these tags.
-#[derive(Debug, PartialEq, Clone)]
-pub enum SubpacketValue<'a> {
+#[derive(Debug, PartialEq, Eq, Hash, Clone)]
+pub enum SubpacketValue {
     /// The subpacket is unknown.
-    Unknown(&'a [u8]),
+    Unknown(Vec<u8>),
     /// The packet is present, but the value is structured incorrectly.
-    Invalid(&'a [u8]),
+    Invalid(Vec<u8>),
 
     /// 4-octet time field
     SignatureCreationTime(Timestamp),
@@ -728,7 +692,7 @@ pub enum SubpacketValue<'a> {
         trust: u8,
     },
     /// Null-terminated regular expression
-    RegularExpression(&'a [u8]),
+    RegularExpression(Vec<u8>),
     /// 1 octet of revocability, 0 for not, 1 for revocable
     Revocable(bool),
     /// 4-octet time field.
@@ -754,7 +718,7 @@ pub enum SubpacketValue<'a> {
     Issuer(KeyID),
     /// The notation has a name and a value, each of
     /// which are strings of octets..
-    NotationData(NotationData<'a>),
+    NotationData(NotationData),
     /// Array of one-octet values
     PreferredHashAlgorithms(Vec<HashAlgorithm>),
     /// Array of one-octet values
@@ -762,22 +726,22 @@ pub enum SubpacketValue<'a> {
     /// N octets of flags
     KeyServerPreferences(KeyServerPreferences),
     /// String (URL)
-    PreferredKeyServer(&'a [u8]),
+    PreferredKeyServer(Vec<u8>),
     /// 1 octet, Boolean
     PrimaryUserID(bool),
     /// String (URL)
-    PolicyURI(&'a [u8]),
+    PolicyURI(Vec<u8>),
     /// N octets of flags
     KeyFlags(KeyFlags),
     /// String
-    SignersUserID(&'a [u8]),
+    SignersUserID(Vec<u8>),
     /// 1 octet of revocation code, N octets of reason string
     ReasonForRevocation {
         /// Machine-readable reason for revocation.
         code: ReasonForRevocation,
 
         /// Human-readable reason for revocation.
-        reason: &'a [u8],
+        reason: Vec<u8>,
     },
     /// N octets of flags
     Features(Features),
@@ -788,7 +752,7 @@ pub enum SubpacketValue<'a> {
         /// Hash algorithm of the target signature.
         hash_algo: HashAlgorithm,
         /// Hash digest of the target signature.
-        digest: &'a [u8],
+        digest: Vec<u8>,
     },
     /// An embedded signature.
     ///
@@ -803,7 +767,7 @@ pub enum SubpacketValue<'a> {
     IntendedRecipient(Fingerprint),
 }
 
-impl<'a> SubpacketValue<'a> {
+impl SubpacketValue {
     /// Returns the length of the serialized value.
     pub fn len(&self) -> usize {
         use self::SubpacketValue::*;
@@ -901,20 +865,30 @@ impl<'a> SubpacketValue<'a> {
 /// Signature subpacket specified by [Section 5.2.3.1 of RFC 4880].
 ///
 /// [Section 5.2.3.1 of RFC 4880]: https://tools.ietf.org/html/rfc4880#section-5.2.3.1
-#[derive(PartialEq, Clone)]
-pub struct Subpacket<'a> {
+#[derive(PartialEq, Eq, Hash, Clone)]
+pub struct Subpacket {
+    /// The length.
+    ///
+    /// In order not to break signatures, we need to be able to
+    /// roundtrip the subpackets, perfectly reproducing all the bits.
+    /// To allow for suboptimal encoding of lenghts, we store the
+    /// length when we parse subpackets.
+    pub(crate) // For serialize/mod.rs.
+    length: SubpacketLength,
     /// Critical flag.
     critical: bool,
     /// Packet type.
     tag: SubpacketTag,
     /// Packet value, must match packet type.
-    value: SubpacketValue<'a>,
+    value: SubpacketValue,
 }
 
-impl<'a> fmt::Debug for Subpacket<'a> {
+impl fmt::Debug for Subpacket {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         let mut s = f.debug_struct("Subpacket");
-
+        if self.length.raw.is_some() || true {
+            s.field("length", &self.length);
+        }
         if self.critical {
             s.field("critical", &self.critical);
         }
@@ -923,18 +897,19 @@ impl<'a> fmt::Debug for Subpacket<'a> {
     }
 }
 
-impl<'a> Subpacket<'a> {
+impl Subpacket {
     /// Creates a new subpacket.
-    pub fn new(value: SubpacketValue<'a>, critical: bool)
-               -> Result<Subpacket<'a>> {
+    pub fn new(value: SubpacketValue, critical: bool)
+               -> Result<Subpacket> {
         Ok(Self::with_tag(value.tag()?, value, critical))
     }
 
     /// Creates a new subpacket with the given tag.
-    pub fn with_tag(tag: SubpacketTag, value: SubpacketValue<'a>,
+    pub fn with_tag(tag: SubpacketTag, value: SubpacketValue,
                     critical: bool)
-               -> Subpacket<'a> {
+               -> Subpacket {
         Subpacket {
+            length: SubpacketLength::from(1 /* Tag */ + value.len() as u32),
             critical,
             tag,
             value,
@@ -952,17 +927,9 @@ impl<'a> Subpacket<'a> {
     }
 
     /// Returns the subpackets value.
-    pub fn value(&self) -> &SubpacketValue<'a> {
+    pub fn value(&self) -> &SubpacketValue {
         &self.value
     }
-
-    /// Returns the length of the serialized subpacket.
-    pub fn len(&self) -> usize {
-        let value_len = self.value.len();
-