refactor(string): improve string security

string/mystring.c

@@ -1,5 +1,4 @@
 #include "mystring.h"
-
 #include <ctype.h>
 #include <stdarg.h>
 #include <stdint.h>
@@ -10,8 +9,9 @@
 
 /* Returns the next power of two of a number */
 static size_t next_power_two(size_t len) {
-	if (len == 0)
+	if (len == 0) {
 		return 1;
+	}
 
 	size_t p = 1;
 	while (p < len) {
@@ -36,11 +36,9 @@ string_s *string_new(const char *text, size_t initial_capacity) {
 	}
 
 	str->size = strlen(text);
-
 	if (initial_capacity != 0 && initial_capacity < (str->size + 1)) {
 		/* Can't allocate with this capacity */
 		free(str);
-
 		return NULL;
 	}
 
@@ -49,14 +47,12 @@ string_s *string_new(const char *text, size_t initial_capacity) {
 		/* Calculate the needed capacity */
 		capacity = next_power_two(str->size + 1);
 	}
-
 	str->capacity = capacity;
 
 	/* Allocate data (text) buffer */
 	str->data = malloc(str->capacity);
 	if (str->data == NULL) {
 		free(str);
-
 		return NULL;
 	}
 
@@ -68,7 +64,6 @@ string_s *string_new(const char *text, size_t initial_capacity) {
 	if (mtx_init(&str->lock, mtx_recursive) != thrd_success) {
 		free(str->data);
 		free(str);
-
 		return NULL;
 	}
 
@@ -88,12 +83,10 @@ int string_append(string_s *string, const char *text) {
 	size_t text_len = strlen(text);
 	if (text_len == 0) {
 		mtx_unlock(&string->lock);
-
 		return 0;
 	}
 
 	size_t new_size = string->size + text_len;
-
 	/* Check if we need to resize */
 	if (new_size + 1 > string->capacity) {
 		size_t new_capacity = next_power_two(new_size + 1);
@@ -101,10 +94,8 @@ int string_append(string_s *string, const char *text) {
 		void *new_data = realloc(string->data, new_capacity);
 		if (!new_data) {
 			mtx_unlock(&string->lock);
-
 			return -1;
 		}
-
 		string->data = new_data;
 		string->capacity = new_capacity;
 	}
@@ -113,7 +104,6 @@ int string_append(string_s *string, const char *text) {
 	memcpy(string->data + string->size, text, text_len);
 	string->size = new_size;
 	string->data[string->size] = '\0';
-
 	mtx_unlock(&string->lock);
 
 	return 0;
@@ -124,37 +114,59 @@ int string_extend(string_s *destination, string_s *source) {
 		return -1;
 	}
 
+	/* Lock both strings. The recursive mutex handles the self-extend case
+	 * (destination == source) without deadlock. */
 	if (mtx_lock(&destination->lock) != thrd_success) {
 		return -1;
 	}
 
-	if (mtx_lock(&source->lock) != thrd_success) {
+	if (destination != source && mtx_lock(&source->lock) != thrd_success) {
 		mtx_unlock(&destination->lock);
 		return -1;
 	}
 
-	size_t need = destination->size + source->size;
-	if (need > destination->capacity) {
+	if (source->size > SIZE_MAX - destination->size - 1) {
+		mtx_unlock(&destination->lock);
+		if (destination != source) {
+			mtx_unlock(&source->lock);
+		}
+		return -1;
+	}
+
+	size_t destination_size = destination->size;
+	size_t source_size = source->size;
+	size_t need = destination_size + source_size;
+	size_t needed_capacity = need + 1;
+
+	if (needed_capacity > destination->capacity) {
 		/* Reallocate destination data buffer */
-		destination->capacity = next_power_two(need);
-		char *tmp = realloc(destination->data, destination->capacity);
+		size_t new_capacity = next_power_two(needed_capacity);
+		char *tmp = realloc(destination->data, new_capacity);
 		if (tmp == NULL) {
 			mtx_unlock(&destination->lock);
-			mtx_unlock(&source->lock);
-
+			if (destination != source) {
+				mtx_unlock(&source->lock);
+			}
 			return -1;
 		}
 		destination->data = tmp;
+		destination->capacity = new_capacity;
+	}
+
+	/* self-extend requires memmove due to overlapping source/destination ranges */
+	if (destination == source) {
+		memmove(destination->data + destination_size, destination->data, source_size);
+	} else {
+		memcpy(destination->data + destination_size, source->data, source_size);
 	}
 
-	/* Copy memory from source data buffer */
-	memcpy(destination->data + destination->size, source->data, source->size);
 	destination->size = need;
 	destination->data[destination->size] = '\0';
 
 	mtx_unlock(&destination->lock);
-	mtx_unlock(&source->lock);
-
+	if (destination != source) {
+		mtx_unlock(&source->lock);
+	}
 	return 0;
 }
 
@@ -168,7 +180,6 @@ void string_free(string_s *string) {
 	}
 
 	mtx_destroy(&string->lock);
-
 	free(string);
 }
 
@@ -182,7 +193,6 @@ size_t string_len(string_s *string) {
 	}
 
 	size_t len = string->size;
-
 	mtx_unlock(&string->lock);
 
 	return len;
@@ -198,7 +208,6 @@ size_t string_cap(string_s *string) {
 	}
 
 	size_t cap = string->capacity;
-
 	mtx_unlock(&string->lock);
 
 	return cap;
@@ -253,7 +262,6 @@ char *string_copy(string_s *string) {
 
 	memcpy(cpy, string->data, string->size);
 	cpy[string->size] = '\0';
-
 	mtx_unlock(&string->lock);
 
 	return cpy;
@@ -264,20 +272,23 @@ int string_compare(string_s *s1, string_s *s2) {
 		return -123;
 	}
 
+	/* Lock both strings. The recursive mutex handles the s1 == s2 case
+	 * without deadlock. */
 	if (mtx_lock(&s1->lock) != thrd_success) {
 		return -123;
 	}
 
-	if (mtx_lock(&s2->lock) != thrd_success) {
+	if (s1 != s2 && mtx_lock(&s2->lock) != thrd_success) {
 		mtx_unlock(&s1->lock);
-
 		return -123;
 	}
 
 	int ret = strcmp(s1->data, s2->data);
 
 	mtx_unlock(&s1->lock);
-	mtx_unlock(&s2->lock);
+	if (s1 != s2) {
+		mtx_unlock(&s2->lock);
+	}
 
 	return ret;
 }
@@ -293,7 +304,6 @@ int string_clear(string_s *string) {
 
 	memset(string->data, 0, string->size);
 	string->size = 0;
-
 	mtx_unlock(&string->lock);
 
 	return 0;
@@ -339,7 +349,6 @@ int string_tolower(string_s *string) {
 static void build_lps(int *lps, const char *substring, size_t sub_len) {
 	int len = 0;
 	size_t i = 1;
-
 	while (i < sub_len) {
 		if (substring[i] == substring[len]) {
 			lps[i++] = ++len;
@@ -362,19 +371,26 @@ int string_find(string_s *string, const char *substring) {
 		return -1;
 	}
 
-	/* Handle empty case */
+	/* Handle empty string case */
 	if (strcmp(string->data, "") == 0) {
 		mtx_unlock(&string->lock);
-
 		return -1;
 	}
 
 	size_t sub_len = strlen(substring);
+	if (sub_len == 0) {
+		mtx_unlock(&string->lock);
+		return 0;
+	}
 
 	int *lps = (int *)malloc(sub_len * sizeof(int));
+	if (lps == NULL) {
+		mtx_unlock(&string->lock);
+		return -1;
+	}
+
 	memset(lps, 0, sub_len * sizeof(int));
 	build_lps(lps, substring, sub_len);
-
 	size_t i = 0; /* string iterator */
 	size_t j = 0; /* substring iterator */
 	while (i < string->size) {
@@ -384,7 +400,6 @@ int string_find(string_s *string, const char *substring) {
 			if (j == sub_len) {
 				free(lps);
 				mtx_unlock(&string->lock);
-
 				return (int)(i - j);
 			}
 		} else {
@@ -397,7 +412,6 @@ int string_find(string_s *string, const char *substring) {
 	}
 
 	free(lps);
-
 	mtx_unlock(&string->lock);
 
 	return -1;
@@ -410,14 +424,22 @@ string_s *string_format(const char *fmt, ...) {
 
 	va_list args;
 	va_start(args, fmt);
-	size_t len = vsnprintf(NULL, 0, fmt, args) + 1;
+	int formatted_len = vsnprintf(NULL, 0, fmt, args);
 	va_end(args);
+	if (formatted_len < 0) {
+		return NULL;
+	}
 
+	size_t len = (size_t)formatted_len + 1;
 	string_s *str = string_new("", len);
+	if (str == NULL) {
+		return NULL;
+	}
 
 	va_start(args, fmt);
 	vsnprintf(str->data, str->capacity, fmt, args);
 	va_end(args);
+	str->size = (size_t)formatted_len;
 
 	return str;
 }
@@ -427,38 +449,44 @@ int string_insert(string_s *string, size_t index, const char *text) {
 		return -1;
 	}
 
-	if (index > string->size) {
-		return -1;
-	}
-
 	if (mtx_lock(&string->lock) != thrd_success) {
 		return -1;
 	}
 
-	size_t text_len = strlen(text);
-	size_t new_size = string->size + text_len;
+	if (index > string->size) {
+		mtx_unlock(&string->lock);
+		return -1;
+	}
 
+	size_t text_len = strlen(text);
+	if (text_len > SIZE_MAX - string->size - 1) {
+		mtx_unlock(&string->lock);
+		return -1;
+	}
+
+	size_t new_size = string->size + text_len;
 	if (new_size + 1 > string->capacity) {
 		/* Reallocate buffer */
 		size_t new_cap = next_power_two(new_size + 1);
 		void *tmp = realloc(string->data, new_cap);
 		if (tmp == NULL) {
 			mtx_unlock(&string->lock);
-
 			return -1;
 		}
 		string->data = tmp;
+		string->capacity = new_cap;
 	}
 
 	/* Shift bytes */
 	memmove((char *)string->data + ((index + text_len) * sizeof(char)),
 			(char *)string->data + (index * sizeof(char)), string->size - index + 1);
+
 	/* Insert new text */
 	memcpy((char *)string->data + (index * sizeof(char)), text, text_len);
 	string->size = new_size;
+
 	/* Ensure null termination */
 	string->data[string->size] = '\0';
-
 	mtx_unlock(&string->lock);
 
 	return 0;
@@ -469,19 +497,21 @@ int string_remove(string_s *string, size_t index, size_t length) {
 		return -1;
 	}
 
-	if (index + length > string->size) {
-		return -1;
-	}
-
 	if (mtx_lock(&string->lock) != thrd_success) {
 		return -1;
 	}
 
+	if (index > string->size || length > string->size - index) {
+		mtx_unlock(&string->lock);
+		return -1;
+	}
+
+	size_t tail_size = string->size - (index + length);
 	memmove((char *)string->data + (index * sizeof(char)),
-			(char *)string->data + ((index + length) * sizeof(char)), string->size - length);
+			(char *)string->data + ((index + length) * sizeof(char)), tail_size + 1);
+
 	string->size -= length;
 	string->data[string->size] = '\0';
-
 	mtx_unlock(&string->lock);
 
 	return 0;
@@ -492,23 +522,30 @@ int string_replace(string_s *string, const char *old_text, const char *new_text)
 		return -1;
 	}
 
+	size_t old_len = strlen(old_text);
+	if (old_len == 0) {
+		return -1;
+	}
+
 	if (mtx_lock(&string->lock) != thrd_success) {
 		return -1;
 	}
 
 	int pos;
-	size_t old_len = strlen(old_text);
-
+	int ret = 0;
 	while (1) {
 		pos = string_find(string, old_text);
 		if (pos == -1) {
 			break;
 		}
-		string_remove(string, pos, old_len);
-		string_insert(string, pos, new_text);
+		if (string_remove(string, (size_t)pos, old_len) != 0 ||
+			string_insert(string, (size_t)pos, new_text) != 0) {
+			ret = -1;
+			break;
+		}
 	}
 
 	mtx_unlock(&string->lock);
 
-	return 0;
+	return ret;
 }