From 9e637f327357019040a23dad928a51dca1fee290 Mon Sep 17 00:00:00 2001
From: Yuxuan Shui <yshuiv7@gmail.com>
Date: Thu, 10 Oct 2024 20:58:42 +0100
Subject: [PATCH] c2: fix resource management for c2_property_value

Previously, if the type of a property changes, or if a number property
went from inline (i.e. value->numbers) to external (i.e. value->array)
or vice versa, we could leak allocated memory, or accessing the wrong
member of a union (i.e. accessing value->array while value->numbers is
active, or vice versa).

Fixes #1350

Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
---
 CHANGELOG.md |  6 ++++++
 src/c2.c     | 49 +++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index d2cdf3eadc..174da0c4c9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,3 +1,9 @@
+# Unreleased
+
+## Bug fixes
+
+* Fix memory corruption that can happen when handling window property changes (#1350)
+
 # 12.2 (2024-Oct-10)
 
 ## Improvements
diff --git a/src/c2.c b/src/c2.c
index 87fb0c63ab..250b3501d7 100644
--- a/src/c2.c
+++ b/src/c2.c
@@ -85,13 +85,16 @@ struct c2_property_value {
 	union {
 		struct {
 			char *string;
+			/// Number of bytes allocated for the string.
+			unsigned int string_capacity;
 		};
 		struct {
 			int64_t numbers[4];
 		};
 		struct {
 			int64_t *array;
-			unsigned int capacity;
+			/// Number of bytes allocated for the array.
+			unsigned int array_capacity;
 		};
 	};
 	/// Number of items if the property is a number type,
@@ -101,6 +104,7 @@ struct c2_property_value {
 		C2_PROPERTY_TYPE_STRING,
 		C2_PROPERTY_TYPE_NUMBER,
 		C2_PROPERTY_TYPE_ATOM,
+		C2_PROPERTY_TYPE_NONE,
 	} type;
 	bool valid;
 	bool needs_update;
@@ -2020,6 +2024,8 @@ c2_window_state_update_one_from_reply(struct c2_state *state,
 	auto len = to_u32_checked(xcb_get_property_value_length(reply));
 	void *data = xcb_get_property_value(reply);
 	bool property_is_string = x_is_type_string(state->atoms, reply->type);
+	unsigned int external_capacity = 0;
+	char *external_storage = NULL;
 	value->needs_update = false;
 	value->valid = false;
 	if (reply->type == XCB_ATOM_NONE) {
@@ -2037,29 +2043,44 @@ c2_window_state_update_one_from_reply(struct c2_state *state,
 		return;
 	}
 
+	if (value->type == C2_PROPERTY_TYPE_STRING) {
+		external_capacity = value->string_capacity;
+		external_storage = value->string;
+	} else if (value->type == C2_PROPERTY_TYPE_NUMBER &&
+	           value->length > ARR_SIZE(value->numbers)) {
+		external_capacity = value->array_capacity;
+		external_storage = (char *)value->array;
+	}
 	log_verbose("Updating property %s, length = %u, format = %d",
 	            get_atom_name_cached(state->atoms, property), len, reply->format);
 	value->valid = true;
 	if (len == 0) {
 		value->length = 0;
-		return;
-	}
-	if (property_is_string) {
+		value->type = C2_PROPERTY_TYPE_NONE;
+	} else if (property_is_string) {
 		bool nul_terminated = ((char *)data)[len - 1] == '\0';
 		value->length = len;
 		value->type = C2_PROPERTY_TYPE_STRING;
 		if (!nul_terminated) {
 			value->length += 1;
 		}
-		value->string = crealloc(value->string, value->length);
+		if (value->length > external_capacity) {
+			value->string = crealloc(external_storage, value->length);
+			value->string_capacity = value->length;
+		} else {
+			value->string = external_storage;
+			value->string_capacity = external_capacity;
+		}
+		external_capacity = 0;
+		external_storage = NULL;
 		memcpy(value->string, data, len);
 		if (!nul_terminated) {
 			value->string[len] = '\0';
 		}
 	} else {
-		size_t step = reply->format / 8;
+		unsigned step = reply->format / 8;
 		bool is_signed = reply->type == XCB_ATOM_INTEGER;
-		value->length = len * 8 / reply->format;
+		value->length = len / step;
 		if (reply->type == XCB_ATOM_ATOM) {
 			value->type = C2_PROPERTY_TYPE_ATOM;
 		} else {
@@ -2068,10 +2089,16 @@ c2_window_state_update_one_from_reply(struct c2_state *state,
 
 		int64_t *storage = value->numbers;
 		if (value->length > ARR_SIZE(value->numbers)) {
-			if (value->capacity < value->length) {
-				value->array = crealloc(value->array, value->length);
-				value->capacity = value->length;
+			const unsigned size = value->length * sizeof(*value->array);
+			if (external_capacity < size) {
+				value->array = (int64_t *)crealloc(external_storage, size);
+				value->array_capacity = size;
+			} else {
+				value->array = (int64_t *)external_storage;
+				value->array_capacity = external_capacity;
 			}
+			external_capacity = 0;
+			external_storage = NULL;
 			storage = value->array;
 		}
 		for (uint32_t i = 0; i < value->length; i++) {
@@ -2101,6 +2128,8 @@ c2_window_state_update_one_from_reply(struct c2_state *state,
 			}
 		}
 	}
+
+	free(external_storage);
 }
 
 static void c2_window_state_update_from_replies(struct c2_state *state,