From e4584a4bbe0565e54ade319889ae5b692836f7ac Mon Sep 17 00:00:00 2001
From: Ana Gelez <ana+github@gelez.xyz>
Date: Sat, 25 May 2024 23:11:03 +0200
Subject: [PATCH] cryptovec: Fix a segmentation fault (#288)

The `CryptoVec::resize` implementation was running into a segmentation
fault in some case. If the capacity of the vector was more than 0, and
that the new allocation failed, the call to
`std::ptr::copy_non_overlapping` would have a null pointer as
destination. This was very easy to trigger by a malicious peer, they
just had to send a packet with an announced size large enough for the
allocation to fail. The code now correctly panics, which would only end
the current thread and not crash the whole application without giving it
a chance to continue running.
---
 cryptovec/src/lib.rs | 41 +++++++++++++++++++++++++++++++----------
 1 file changed, 31 insertions(+), 10 deletions(-)

diff --git a/cryptovec/src/lib.rs b/cryptovec/src/lib.rs
index 8ecd1f0d..256b7fc2 100644
--- a/cryptovec/src/lib.rs
+++ b/cryptovec/src/lib.rs
@@ -248,7 +248,15 @@ impl CryptoVec {
                 let next_capacity = size.next_power_of_two();
                 let old_ptr = self.p;
                 let next_layout = std::alloc::Layout::from_size_align_unchecked(next_capacity, 1);
-                self.p = std::alloc::alloc_zeroed(next_layout);
+                let new_ptr = std::alloc::alloc_zeroed(next_layout);
+                if new_ptr.is_null() {
+                    #[allow(clippy::panic)]
+                    {
+                        panic!("Realloc failed, pointer = {:?} {:?}", self, size)
+                    }
+                }
+
+                self.p = new_ptr;
                 mlock(self.p, next_capacity);
 
                 if self.capacity > 0 {
@@ -261,15 +269,8 @@ impl CryptoVec {
                     std::alloc::dealloc(old_ptr, layout);
                 }
 
-                if self.p.is_null() {
-                    #[allow(clippy::panic)]
-                    {
-                        panic!("Realloc failed, pointer = {:?} {:?}", self, size)
-                    }
-                } else {
-                    self.capacity = next_capacity;
-                    self.size = size;
-                }
+                self.capacity = next_capacity;
+                self.size = size;
             }
         }
     }
@@ -429,3 +430,23 @@ impl Drop for CryptoVec {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    // If `resize` is called with a size that is too large to be allocated, it
+    // should panic, and not segfault or fail silently.
+    #[test]
+    fn large_resize_panics() {
+        let result = std::panic::catch_unwind(|| {
+            let mut vec = CryptoVec::new();
+            // Write something into the vector, so that there is something to
+            // copy when reallocating, to test all code paths.
+            vec.push(42);
+
+            vec.resize(1_000_000_000_000)
+        });
+        assert!(result.is_err());
+    }
+}