From: Florian Forster <octo@collectd.org>
Date: Mon, 29 Jun 2015 15:32:44 +0000 (+0200)
Subject: src/daemon/utils_subst.c: Sanitize buffer offsets better.
X-Git-Tag: collectd-5.6.0~660
X-Git-Url: https://git.verplant.org/?a=commitdiff_plain;h=e1b93f596067106e7c29c263b8bb4d0b51718d9d;p=collectd.git

src/daemon/utils_subst.c: Sanitize buffer offsets better.

The previous implementation broke when off1 / off2 were outside of "string"
but within "buflen". It also had problems if the replacement string was
too long. This new implementation truncates the buffer as expected and
is properly tested.
---

diff --git a/src/daemon/Makefile.am b/src/daemon/Makefile.am
index 00a8fd73..8275eee8 100644
--- a/src/daemon/Makefile.am
+++ b/src/daemon/Makefile.am
@@ -89,8 +89,8 @@ else
 collectd_LDADD += -loconfig
 endif
 
-check_PROGRAMS = test_common test_utils_avltree test_utils_heap
-TESTS = test_common test_utils_avltree test_utils_heap
+check_PROGRAMS = test_common test_utils_avltree test_utils_heap test_utils_subst
+TESTS = test_common test_utils_avltree test_utils_heap test_utils_subst
 
 test_common_SOURCES = common_test.c ../testing.h
 test_common_LDADD = libcommon.la libplugin_mock.la $(COMMON_LIBS)
@@ -100,3 +100,7 @@ test_utils_avltree_LDADD = libavltree.la $(COMMON_LIBS)
 
 test_utils_heap_SOURCES = utils_heap_test.c ../testing.h
 test_utils_heap_LDADD = libheap.la $(COMMON_LIBS)
+
+test_utils_subst_SOURCES = utils_subst_test.c ../testing.h \
+			   utils_subst.c utils_subst.h
+test_utils_subst_LDADD = libcommon.la libplugin_mock.la $(COMMON_LIBS)
diff --git a/src/daemon/utils_subst.c b/src/daemon/utils_subst.c
index 2f28eb9b..43634df2 100644
--- a/src/daemon/utils_subst.c
+++ b/src/daemon/utils_subst.c
@@ -31,33 +31,60 @@
 #include "collectd.h"
 #include "common.h"
 
-char *subst (char *buf, size_t buflen, const char *string, int off1, int off2,
+char *subst (char *buf, size_t buflen, const char *string, size_t off1, size_t off2,
 		const char *replacement)
 {
-	char  *buf_ptr = buf;
-	size_t len     = buflen;
+	char *out = buf;
 
-	if ((NULL == buf) || (0 >= buflen) || (NULL == string)
-			|| (0 > off1) || (0 > off2) || (off1 > off2)
-			|| (NULL == replacement))
+	char const *front;
+	char const *back;
+	size_t front_len;
+	size_t replacement_len;
+	size_t back_len;
+
+	if ((NULL == buf) || (0 == buflen) || (NULL == string) || (NULL == replacement))
 		return NULL;
 
-	sstrncpy (buf_ptr, string,
-			((size_t)off1 + 1 > buflen) ? buflen : (size_t)off1 + 1);
-	buf_ptr += off1;
-	len     -= off1;
+	size_t string_len = strlen (string);
+	if ((off1 > string_len) || (off2 > string_len) || (off1 > off2))
+		return NULL;
 
-	if (0 >= len)
-		return buf;
+	front = string;
+	back = string + off2;
+	front_len = off1;
+	replacement_len = strlen (replacement);
+	back_len = strlen (back);
+
+	if (front_len >= buflen) {
+		front_len = buflen - 1;
+		replacement_len = 0;
+		back_len = 0;
+	} else if ((front_len + replacement_len) >= buflen) {
+		replacement_len = buflen - (front_len + 1);
+		back_len = 0;
+	} else if ((front_len + replacement_len + back_len) >= buflen) {
+		back_len = buflen - (front_len + replacement_len + 1);
+	} else {
+		buflen = front_len + replacement_len + back_len + 1;
+	}
+	assert ((front_len + replacement_len + back_len) == (buflen - 1));
 
-	sstrncpy (buf_ptr, replacement, len);
-	buf_ptr += strlen (replacement);
-	len     -= strlen (replacement);
+	if (front_len != 0) {
+		sstrncpy (out, front, front_len + 1);
+		out += front_len;
+	}
 
-	if (0 >= len)
-		return buf;
+	if (replacement_len != 0) {
+		sstrncpy (out, replacement, replacement_len + 1);
+		out += replacement_len;
+	}
 
-	sstrncpy (buf_ptr, string + off2, len);
+	if (back_len != 0) {
+		sstrncpy (out, back, back_len + 1);
+		out += back_len;
+	}
+
+	out[0] = 0;
 	return buf;
 } /* subst */
 
@@ -87,7 +114,6 @@ char *asubst (const char *string, int off1, int off2, const char *replacement)
 char *subst_string (char *buf, size_t buflen, const char *string,
 		const char *needle, const char *replacement)
 {
-	char *temp;
 	size_t needle_len;
 	size_t i;
 
@@ -95,19 +121,13 @@ char *subst_string (char *buf, size_t buflen, const char *string,
 			|| (needle == NULL) || (replacement == NULL))
 		return (NULL);
 
-	temp = (char *) malloc (buflen);
-	if (temp == NULL)
-	{
-		ERROR ("subst_string: malloc failed.");
-		return (NULL);
-	}
-
 	needle_len = strlen (needle);
-	strncpy (buf, string, buflen);
+	sstrncpy (buf, string, buflen);
 
 	/* Limit the loop to prevent endless loops. */
 	for (i = 0; i < buflen; i++)
 	{
+		char temp[buflen];
 		char *begin_ptr;
 		size_t begin;
 
@@ -141,7 +161,6 @@ char *subst_string (char *buf, size_t buflen, const char *string,
 				i, string, needle, replacement);
 	}
 
-	sfree (temp);
 	return (buf);
 } /* char *subst_string */
 
diff --git a/src/target_replace.c b/src/target_replace.c
index a85eced3..bd8f9e5c 100644
--- a/src/target_replace.c
+++ b/src/target_replace.c
@@ -190,7 +190,7 @@ static int tr_action_invoke (tr_action_t *act_head, /* {{{ */
     }
 
     subst_status = subst (temp, sizeof (temp), buffer,
-        matches[0].rm_so, matches[0].rm_eo, act->replacement);
+        (size_t) matches[0].rm_so, (size_t) matches[0].rm_eo, act->replacement);
     if (subst_status == NULL)
     {
       ERROR ("Target `replace': subst (buffer = %s, start = %zu, end = %zu, "