From 5c6e4d35b0195e97fcb3dbfdd4b20df6dcbad13d Mon Sep 17 00:00:00 2001
From: SmallJoker <SmallJoker@users.noreply.github.com>
Date: Mon, 21 Apr 2025 12:33:19 +0200
Subject: [PATCH] Client: protect against circular attachments (#16038)

The server already includes such check. There must be a desync issue that causes
an ID mismatch, resulting in client crashes. Any such crash must be prevented.
---
 src/client/content_cao.cpp | 26 ++++++++++++++++++++++++++
 src/server/unit_sao.cpp    |  6 ++++--
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/src/client/content_cao.cpp b/src/client/content_cao.cpp
index 02e980cab..e0ac5fff0 100644
--- a/src/client/content_cao.cpp
+++ b/src/client/content_cao.cpp
@@ -411,6 +411,32 @@ void GenericCAO::setChildrenVisible(bool toset)
 void GenericCAO::setAttachment(object_t parent_id, const std::string &bone,
 		v3f position, v3f rotation, bool force_visible)
 {
+	// Do checks to avoid circular references
+	// See similar check in `UnitSAO::setAttachment` (but with different types).
+	{
+		auto *obj = m_env->getActiveObject(parent_id);
+		if (obj == this) {
+			assert(false);
+			return;
+		}
+		bool problem = false;
+		if (obj) {
+			// The chain of wanted parent must not refer or contain "this"
+			for (obj = obj->getParent(); obj; obj = obj->getParent()) {
+				if (obj == this) {
+					problem = true;
+					break;
+				}
+			}
+		}
+		if (problem) {
+			warningstream << "Network or mod bug: "
+				<< "Attempted to attach object " << m_id << " to parent "
+				<< parent_id << " but former is an (in)direct parent of latter." << std::endl;
+			return;
+		}
+	}
+
 	const auto old_parent = m_attachment_parent_id;
 	m_attachment_parent_id = parent_id;
 	m_attachment_bone = bone;
diff --git a/src/server/unit_sao.cpp b/src/server/unit_sao.cpp
index d7fff69bf..35c92063d 100644
--- a/src/server/unit_sao.cpp
+++ b/src/server/unit_sao.cpp
@@ -128,8 +128,9 @@ void UnitSAO::setAttachment(const object_t new_parent, const std::string &bone,
 	};
 
 	// Do checks to avoid circular references
+	// See similar check in `GenericCAO::setAttachment` (but with different types).
 	{
-		auto *obj = new_parent ? m_env->getActiveObject(new_parent) : nullptr;
+		auto *obj = m_env->getActiveObject(new_parent);
 		if (obj == this) {
 			assert(false);
 			return;
@@ -145,7 +146,8 @@ void UnitSAO::setAttachment(const object_t new_parent, const std::string &bone,
 			}
 		}
 		if (problem) {
-			warningstream << "Mod bug: Attempted to attach object " << m_id << " to parent "
+			warningstream << "Mod bug: "
+				<< "Attempted to attach object " << m_id << " to parent "
 				<< new_parent << " but former is an (in)direct parent of latter." << std::endl;
 			return;
 		}