fix: remove potential panic in SignedRelocateDetails

src/agreement/proposal.rs

@@ -154,43 +154,45 @@ mod tests {
         // Proposal::SectionInfo
         let (elders_info, _) = section::test_utils::gen_elders_info(Default::default(), 4);
         let proposal = Proposal::SectionInfo(elders_info.clone());
-        verify_serialize_for_signing(&proposal, &elders_info);
+        verify_serialize_for_signing(&proposal, &elders_info)?;
 
         // Proposal::OurElders
         let new_sk = bls::SecretKey::random();
         let new_pk = new_sk.public_key();
         let proven_elders_info = agreement::test_utils::proven(&new_sk, elders_info)?;
         let proposal = Proposal::OurElders(proven_elders_info);
-        verify_serialize_for_signing(&proposal, &new_pk);
+        verify_serialize_for_signing(&proposal, &new_pk)?;
 
         // Proposal::TheirKey
         let prefix = gen_prefix();
         let key = bls::SecretKey::random().public_key();
         let proposal = Proposal::TheirKey { prefix, key };
-        verify_serialize_for_signing(&proposal, &(prefix, key));
+        verify_serialize_for_signing(&proposal, &(prefix, key))?;
 
         // Proposal::TheirKnowledge
         let prefix = gen_prefix();
         let key = bls::SecretKey::random().public_key();
         let proposal = Proposal::TheirKnowledge { prefix, key };
-        verify_serialize_for_signing(&proposal, &(prefix, key));
+        verify_serialize_for_signing(&proposal, &(prefix, key))?;
 
         Ok(())
     }
 
     // Verify that `SignableView(proposal)` serializes the same as `should_serialize_as`.
-    fn verify_serialize_for_signing<T>(proposal: &Proposal, should_serialize_as: &T)
+    fn verify_serialize_for_signing<T>(proposal: &Proposal, should_serialize_as: &T) -> Result<()>
     where
         T: Serialize + Debug,
     {
-        let actual = bincode::serialize(&SignableView(proposal)).unwrap();
-        let expected = bincode::serialize(should_serialize_as).unwrap();
+        let actual = bincode::serialize(&SignableView(proposal))?;
+        let expected = bincode::serialize(should_serialize_as)?;
 
         assert_eq!(
             actual, expected,
             "expected SignableView({:?}) to serialize same as {:?}, but didn't",
             proposal, should_serialize_as
-        )
+        );
+
+        Ok(())
     }
 
     fn gen_prefix() -> Prefix {

src/relocation.rs

@@ -123,22 +123,21 @@ impl SignedRelocateDetails {
         }
     }
 
-    // FIXME: need a non-panicking version of this, because when we receive it from another node,
-    // we can't be sure it's well formed.
-    pub fn relocate_details(&self) -> &RelocateDetails {
+    pub fn relocate_details(&self) -> Result<&RelocateDetails, Error> {
         if let Variant::Relocate(details) = &self.signed_msg.variant() {
-            details
+            Ok(details)
         } else {
-            panic!("SignedRelocateDetails always contain Variant::Relocate")
+            error!("SignedRelocateDetails does not contain Variant::Relocate");
+            Err(Error::InvalidMessage)
         }
     }
 
     pub fn signed_msg(&self) -> &Message {
         &self.signed_msg
     }
 
-    pub fn destination(&self) -> &XorName {
-        &self.relocate_details().destination
+    pub fn destination(&self) -> Result<&XorName, Error> {
+        Ok(&self.relocate_details()?.destination)
     }
 }
 
@@ -179,8 +178,13 @@ impl RelocatePayload {
     }
 
     pub fn verify_identity(&self, new_name: &XorName) -> bool {
-        let pub_key = if let Ok(pub_key) = crypto::pub_key(&self.details.relocate_details().pub_id)
-        {
+        let details = if let Ok(details) = self.details.relocate_details() {
+            details
+        } else {
+            return false;
+        };
+
+        let pub_key = if let Ok(pub_key) = crypto::pub_key(&details.pub_id) {
             pub_key
         } else {
             return false;
@@ -191,7 +195,7 @@ impl RelocatePayload {
             .is_ok()
     }
 
-    pub fn relocate_details(&self) -> &RelocateDetails {
+    pub fn relocate_details(&self) -> Result<&RelocateDetails, Error> {
         self.details.relocate_details()
     }
 }

src/routing/bootstrap.rs

@@ -124,7 +124,7 @@ impl<'a> State<'a> {
             .await?;
 
         let relocate_payload = if let Some(details) = relocate_details {
-            Some(self.process_relocation(&prefix, details))
+            Some(self.process_relocation(&prefix, details)?)
         } else {
             None
         };
@@ -195,9 +195,10 @@ impl<'a> State<'a> {
 
         debug!("Sending GetSectionQuery to {:?}", recipients);
 
-        let destination = relocate_details
-            .map(|details| *details.destination())
-            .unwrap_or_else(|| self.node.name());
+        let destination = match relocate_details {
+            Some(details) => *details.destination()?,
+            None => self.node.name(),
+        };
 
         let message = SectionInfoMsg::GetSectionQuery(destination);
 
@@ -213,9 +214,10 @@ impl<'a> State<'a> {
         &mut self,
         relocate_details: Option<&SignedRelocateDetails>,
     ) -> Result<(GetSectionResponse, SocketAddr)> {
-        let destination = relocate_details
-            .map(|details| *details.destination())
-            .unwrap_or_else(|| self.node.name());
+        let destination = match relocate_details {
+            Some(details) => *details.destination()?,
+            None => self.node.name(),
+        };
 
         while let Some((message, sender)) = self.recv_rx.next().await {
             match message {
@@ -257,16 +259,16 @@ impl<'a> State<'a> {
         &mut self,
         prefix: &Prefix,
         relocate_details: SignedRelocateDetails,
-    ) -> RelocatePayload {
+    ) -> Result<RelocatePayload> {
         // We are relocating so we need to change our name.
         // Use a name that will match the destination even after multiple splits
         let extra_split_count = 3;
         let name_prefix = Prefix::new(
             prefix.bit_count() + extra_split_count,
-            *relocate_details.destination(),
+            *relocate_details.destination()?,
         );
 
-        let age = relocate_details.relocate_details().age;
+        let age = relocate_details.relocate_details()?.age;
         let new_keypair = crypto::gen_keypair(&name_prefix.range_inclusive(), age);
         let new_name = crypto::name(&new_keypair.public);
         let relocate_payload =
@@ -275,7 +277,7 @@ impl<'a> State<'a> {
         info!("Changing name to {}", new_name);
         self.node = Node::new(new_keypair, self.node.addr);
 
-        relocate_payload
+        Ok(relocate_payload)
     }
 
     // Send `JoinRequest` and wait for the response. If the response is `Rejoin`, repeat with the
@@ -463,7 +465,7 @@ impl<'a> State<'a> {
                     }
 
                     let trusted_key = if let Some(payload) = relocate_payload {
-                        Some(&payload.relocate_details().destination_key)
+                        Some(&payload.relocate_details()?.destination_key)
                     } else {
                         None
                     };

src/routing/core.rs

@@ -682,7 +682,7 @@ impl Core {
             Variant::Relocate(_) => {
                 if msg.src().is_section() {
                     let signed_relocate = SignedRelocateDetails::new(msg)?;
-                    Ok(self.handle_relocate(signed_relocate).into_iter().collect())
+                    Ok(self.handle_relocate(signed_relocate)?.into_iter().collect())
                 } else {
                     Err(Error::InvalidSrcLocation)
                 }
@@ -1073,22 +1073,22 @@ impl Core {
         self.update_state(snapshot)
     }
 
-    fn handle_relocate(&mut self, details: SignedRelocateDetails) -> Option<Command> {
-        if details.relocate_details().pub_id != self.node.name() {
+    fn handle_relocate(&mut self, details: SignedRelocateDetails) -> Result<Option<Command>> {
+        if details.relocate_details()?.pub_id != self.node.name() {
             // This `Relocate` message is not for us - it's most likely a duplicate of a previous
             // message that we already handled.
-            return None;
+            return Ok(None);
         }
 
         debug!(
             "Received Relocate message to join the section at {}",
-            details.relocate_details().destination
+            details.relocate_details()?.destination
         );
 
         match self.relocate_state {
             Some(RelocateState::InProgress(_)) => {
                 trace!("Ignore Relocate - relocation already in progress");
-                return None;
+                return Ok(None);
             }
             Some(RelocateState::Delayed(_)) => (),
             None => {
@@ -1109,11 +1109,11 @@ impl Core {
             .copied()
             .collect();
 
-        Some(Command::Relocate {
+        Ok(Some(Command::Relocate {
             bootstrap_addrs,
             details,
             message_rx,
-        })
+        }))
     }
 
     fn handle_relocate_promise(
@@ -1223,8 +1223,7 @@ impl Core {
                     return Ok(vec![]);
                 }
 
-                // FIXME: this might panic if the payload is malformed.
-                let details = payload.relocate_details();
+                let details = payload.relocate_details()?;
 
                 if !self.section.prefix().matches(&details.destination) {
                     debug!(