Fix two server crashes: Team Objective stack overflow under --desync-patch, access violation in the combat hooks

src/resolvers/gameplay.rs

@@ -3,15 +3,18 @@ use std::os::raw::c_void;
 use crate::events::models::{CombatActor, Damage, DamageSource, GameEvent, Kill};
 use crate::features::events::EVENT_SYSTEM;
 use crate::game::chivalry2::{AController, APlayerState, ATBLCharacter, FDeathDamageTakenEvent, FDamageTakenEvent, PlayerFlags};
+use crate::tools::hook_globals::globals;
 use crate::ue::{UObject, UStruct};
 
 fn object_class_name(ptr: *mut c_void) -> Option<String> {
+    if !is_live_object(ptr) { return None; }
     let obj = unsafe { (ptr as *const UObject).as_ref() }?;
     let class = unsafe { obj.uobject_base_utility.uobject_base.class_private.as_ref() }?;
     Some(class.ustruct.ufield.uobject.uobject_base_utility.uobject_base.name_private.to_string())
 }
 
 fn object_name(ptr: *mut c_void) -> Option<String> {
+    if !is_live_object(ptr) { return None; }
     let obj = unsafe { (ptr as *const UObject).as_ref() }?;
     Some(obj.uobject_base_utility.uobject_base.name_private.to_string())
 }
@@ -23,16 +26,36 @@ fn object_display_name(ptr: *mut c_void) -> Option<String> {
         .filter(|name| !name.trim().is_empty())
 }
 
+/// Cheap pre-filter: non-null, 8-byte aligned and inside the canonical user-mode range. Not proof
+/// that an object lives there (0x100000000 passes); use is_live_object before reading through a
+/// pointer.
+fn looks_like_pointer(p: *const c_void) -> bool {
+    let v = p as usize;
+    v >= 0x10000 && v < 0x0000_7FFF_FFFF_0000 && v % 8 == 0
+}
+
+/// True when `p` is a live UObject according to GUObjectArray (FUObjectArray::is_live): catches
+/// unmapped garbage and dangling pointers to destroyed objects alike.
+fn is_live_object(p: *const c_void) -> bool {
+    looks_like_pointer(p) && unsafe { globals().guobject_array_unchecked() }.is_live(p.cast())
+}
+
 fn object_inherits_from(ptr: *mut c_void, needle: &str) -> bool {
+    if !is_live_object(ptr) { return false; }
     let mut curr = unsafe { (ptr as *const UObject).as_ref() }
         .and_then(|o| unsafe { o.uobject_base_utility.uobject_base.class_private.as_ref() })
         .map(|c| &c.ustruct as *const UStruct);
 
+    let mut depth = 0;
     while let Some(s) = unsafe { curr.and_then(|p| p.as_ref()) } {
-        if s.ufield.uobject.uobject_base_utility.uobject_base.name_private.to_string().contains(needle) {
+        // Exact class-name match: a substring match would let "Character" accept classes such as
+        // CharacterDeathcam_C, which do not have the ATBLCharacter layout.
+        if s.ufield.uobject.uobject_base_utility.uobject_base.name_private.to_string() == needle {
             return true;
         }
-        curr = (!s.super_struct.is_null()).then_some(s.super_struct);
+        depth += 1;
+        if depth > 32 { return false; }
+        curr = looks_like_pointer(s.super_struct.cast()).then_some(s.super_struct);
     }
     false
 }
@@ -57,7 +80,13 @@ fn fallback_combat_actor(ptr: *mut c_void) -> Option<CombatActor> {
     Some(CombatActor::new(normalize_display_name(&obj), is_bot))
 }
 
-fn combat_actor_from_player_state(player_state: *mut APlayerState) -> Option<CombatActor> {
+/// `via` names the field the pointer came from, for the warning when it is not a live object.
+fn combat_actor_from_player_state(player_state: *mut APlayerState, via: &str) -> Option<CombatActor> {
+    if player_state.is_null() { return None; }
+    if !is_live_object(player_state.cast()) {
+        crate::swarn!(f; "ignoring PlayerState {:p} from {}: not a live object", player_state, via);
+        return None;
+    }
     let ps = unsafe { player_state.as_ref() }?;
     let name = ps.player_name_private.copy_to_string().ok()?;
     (!name.trim().is_empty()).then(|| {
@@ -67,28 +96,40 @@ fn combat_actor_from_player_state(player_state: *mut APlayerState) -> Option<Com
 
 fn combat_actor_from_actor(ptr: *mut c_void) -> Option<CombatActor> {
     if ptr.is_null() { return None; }
+    if !is_live_object(ptr) {
+        crate::swarn!(f; "ignoring combat actor {:p}: not a live object", ptr);
+        return None;
+    }
 
-    if object_inherits_from(ptr, "TBLCharacter") || object_inherits_from(ptr, "Character") {
+    let is_tbl_character = object_inherits_from(ptr, "TBLCharacter");
+    if is_tbl_character || object_inherits_from(ptr, "Character") {
+        // The APawn fields are valid on any Character; the cast to ATBLCharacter is only read
+        // further for LastPlayerState, which exists on ATBLCharacter alone and lies past the end
+        // of a plain ACharacter.
         let char = unsafe { (ptr as *mut ATBLCharacter).as_ref() }?;
         let pawn = &char.base_tbl_character_base.base_character.base_pawn;
 
-        return combat_actor_from_player_state(pawn.player_state as *mut APlayerState)
+        return combat_actor_from_player_state(pawn.player_state as *mut APlayerState, "Pawn.PlayerState")
             .or_else(|| {
+                if !is_live_object(pawn.controller) { return None; }
                 let controller = unsafe { pawn.controller.cast::<AController>().as_ref() }?;
-                combat_actor_from_player_state(controller.player_state)
+                combat_actor_from_player_state(controller.player_state, "Pawn.Controller.PlayerState")
+            })
+            .or_else(|| {
+                if !is_tbl_character { return None; }
+                combat_actor_from_player_state(char.last_player_state as *mut APlayerState, "TBLCharacter.LastPlayerState")
             })
-            .or_else(|| combat_actor_from_player_state(char.last_player_state as *mut APlayerState))
             .or_else(|| fallback_combat_actor(ptr));
     }
 
-    if object_inherits_from(ptr, "Controller") {
+    if object_inherits_from(ptr, "Controller") || object_inherits_from(ptr, "PlayerController") || object_inherits_from(ptr, "AIController") || object_inherits_from(ptr, "TBLPlayerController") {
         let controller = unsafe { (ptr as *mut AController).as_ref() }?;
-        return combat_actor_from_player_state(controller.player_state)
+        return combat_actor_from_player_state(controller.player_state, "Controller.PlayerState")
             .or_else(|| fallback_combat_actor(ptr));
     }
 
     if object_inherits_from(ptr, "PlayerState") {
-        return combat_actor_from_player_state(ptr.cast())
+        return combat_actor_from_player_state(ptr.cast(), "PlayerState")
             .or_else(|| fallback_combat_actor(ptr));
     }
 

src/ue/memory.rs

@@ -0,0 +1,25 @@
+use windows::Win32::System::Diagnostics::Debug::ReadProcessMemory;
+use windows::Win32::System::Threading::GetCurrentProcess;
+
+/// Copies a `T` from `ptr` without dereferencing it: ReadProcessMemory on our own process fails
+/// cleanly on unmapped or protected memory where a plain read raises an access violation. For
+/// values read through pointers that may be dangling or garbage (fields of game objects that
+/// were never validated).
+pub fn try_read<T: Copy>(ptr: *const T) -> Option<T> {
+    if ptr.is_null() {
+        return None;
+    }
+    let size = std::mem::size_of::<T>();
+    let mut out = std::mem::MaybeUninit::<T>::uninit();
+    let mut bytes_read = 0usize;
+    let result = unsafe {
+        ReadProcessMemory(
+            GetCurrentProcess(),
+            ptr.cast(),
+            out.as_mut_ptr().cast(),
+            size,
+            Some(&mut bytes_read),
+        )
+    };
+    (result.is_ok() && bytes_read == size).then(|| unsafe { out.assume_init() })
+}

src/ue/mod.rs

@@ -3,6 +3,7 @@ mod array;
 pub mod kismet;
 mod malloc;
 mod map;
+mod memory;
 mod name;
 mod object;
 pub mod object_array;
@@ -12,6 +13,7 @@ pub mod sync;
 pub use array::*;
 pub use malloc::*;
 pub use map::*;
+pub use memory::*;
 pub use name::*;
 pub use object::*;
 pub use string::*;

src/ue/object_array.rs

@@ -35,6 +35,28 @@ impl FUObjectArray {
         CriticalSectionGuard::lock(&self.obj_objects_critical, &self.obj_objects)
     }
 
+    /// True when `obj` is a live object: the InternalIndex stored in it, read without faulting,
+    /// names a slot that still holds this very pointer, and that slot is neither pending kill nor
+    /// unreachable. A dangling or garbage pointer fails because its memory is unreadable, or its
+    /// slot is out of range, empty or holds another object. Reads the chunk table without the
+    /// lock, as allocate_serial_number below does; meant for the game thread.
+    pub fn is_live(&self, obj: *const UObjectBase) -> bool {
+        const UNREACHABLE: i32 = 1 << 28;
+        const PENDING_KILL: i32 = 1 << 29;
+        if obj.is_null() {
+            return false;
+        }
+        let index_ptr = (obj as *const u8)
+            .wrapping_add(std::mem::offset_of!(UObjectBase, internal_index)) as *const i32;
+        let Some(index) = super::try_read(index_ptr) else { return false };
+        let objects = unsafe { &*self.obj_objects.get() };
+        if index < 0 || index >= objects.num_elements || objects.max_chunks <= 0 {
+            return false;
+        }
+        let item = objects.item(index);
+        std::ptr::eq(item.object, obj) && item.flags & (UNREACHABLE | PENDING_KILL) == 0
+    }
+
     pub fn allocate_serial_number(&self, index: ObjectIndex) -> i32 {
         let objects = unsafe { &*self.obj_objects.get() };
         let item = objects.item(index);

src/ue/string.rs

@@ -19,6 +19,7 @@ pub enum FStringCopyError {
     MemoryReadFailed,
     PartialRead,
     Utf16DecodeFailed,
+    ImplausibleHeader,
 }
 
 impl FStringCopyError {
@@ -29,6 +30,7 @@ impl FStringCopyError {
             FStringCopyError::MemoryReadFailed => "FString backing memory copy failed",
             FStringCopyError::PartialRead => "FString backing memory copy was partial",
             FStringCopyError::Utf16DecodeFailed => "FString UTF-16 decode failed",
+            FStringCopyError::ImplausibleHeader => "FString header had an impossible length",
         }
     }
 }
@@ -38,12 +40,27 @@ impl TArray<u16> {
     /// in the FString's backing buffer, which is not safe to do in a multithreaded environment.
     /// This function copies the buffer to a new string, which is safe to do in a multithreaded
     /// environment.
+    ///
+    /// The header (data pointer, length, capacity) is read the same way as the buffer, so an
+    /// FString inside a dangling or garbage object returns an error instead of raising an access
+    /// violation. A header whose length is negative, exceeds its capacity or is implausibly large
+    /// is rejected before anything is allocated.
     pub fn copy_to_string(&self) -> Result<String, FStringCopyError> {
-        let utf16_len = self.len();
-        let utf16_ptr = self.as_ptr();
-        if utf16_len == 0 || utf16_ptr.is_null() {
+        #[derive(Clone, Copy)]
+        #[repr(C)]
+        struct Header { data: *const u16, num: i32, max: i32 }
+        const MAX_CODE_UNITS: i32 = 1 << 20;
+
+        let header = super::try_read(self as *const Self as *const Header)
+            .ok_or(FStringCopyError::MemoryReadFailed)?;
+        if header.num == 0 || header.data.is_null() {
             return Err(FStringCopyError::NullOrEmptyBuffer);
         }
+        if header.num < 0 || header.num > header.max || header.num > MAX_CODE_UNITS {
+            return Err(FStringCopyError::ImplausibleHeader);
+        }
+        let utf16_len = header.num as usize;
+        let utf16_ptr = header.data;
 
         let byte_len = utf16_len
             .checked_mul(std::mem::size_of::<u16>())