[PATCH 06/10] opreport: fix buffer overflows in xml generator

[Robert R. <rob...@am...>]

When running opreport for heavy loads I got some data that was
producing large xml tags causing a buffer overflow. Looking at the
code this behaviour was clear. The function interface is broken by
design:

-static void xml_do_arch_specific_event_help(struct op_event const * event,
-                                       char * buffer)
+static void xml_do_arch_specific_event_help(struct op_event const *event,
+                                       char *buffer, size_t size)

It requires a buffer to write to, but does not provide the size of
this buffer. Thus it is not possible to avoid buffer overflows since
the size of xml tags may vary and depends on the input data. This
patch fixes this.

Signed-off-by: Robert Richter <rob...@am...>
---
 ChangeLog                |    7 ++
 libop/op_xml_events.c    |   65 ++++++++-------
 libop/op_xml_out.c       |  197 +++++++++++++++++++++++++++-------------------
 libop/op_xml_out.h       |   10 +-
 libutil++/xml_output.cpp |   18 ++---
 5 files changed, 169 insertions(+), 128 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 5c32677..ac4736d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,12 @@
 2009-08-11  Robert Richter <rob...@am...>
 
+	* libop/op_xml_events.c:
+	* libop/op_xml_out.c:
+	* libop/op_xml_out.h:
+	* libutil++/xml_output.cpp: fix buffer overflows in xml generator
+
+2009-08-11  Robert Richter <rob...@am...>
+
 	* utils/opcontrol: fix, stop only if enabled
 
 2009-08-11  Robert Richter <rob...@am...>
diff --git a/libop/op_xml_events.c b/libop/op_xml_events.c
index 5b9ac7d..63f21d2 100644
--- a/libop/op_xml_events.c
+++ b/libop/op_xml_events.c
@@ -17,37 +17,37 @@
 
 static op_cpu cpu_type;
 #define MAX_BUFFER 4096
+static char buffer[MAX_BUFFER];
+
 void open_xml_events(char const * title, char const * doc, op_cpu the_cpu_type)
 {
 	char const * schema_version = "1.0";
-	char buffer[MAX_BUFFER];
 
 	buffer[0] = '\0';
 	cpu_type = the_cpu_type;
-	open_xml_element(HELP_EVENTS, 0, buffer);
-	open_xml_element(HELP_HEADER, 1, buffer);
-	init_xml_str_attr(HELP_TITLE, title, buffer);
-	init_xml_str_attr(SCHEMA_VERSION, schema_version, buffer);
-	init_xml_str_attr(HELP_DOC, doc, buffer);
-	close_xml_element(NONE, 0, buffer);
+	open_xml_element(HELP_EVENTS, 0, buffer, MAX_BUFFER);
+	open_xml_element(HELP_HEADER, 1, buffer, MAX_BUFFER);
+	init_xml_str_attr(HELP_TITLE, title, buffer, MAX_BUFFER);
+	init_xml_str_attr(SCHEMA_VERSION, schema_version, buffer, MAX_BUFFER);
+	init_xml_str_attr(HELP_DOC, doc, buffer, MAX_BUFFER);
+	close_xml_element(NONE, 0, buffer, MAX_BUFFER);
 	printf("%s", buffer);
 }
 
 void close_xml_events(void)
 {
-	char buffer[MAX_BUFFER];
-
 	buffer[0] = '\0';
-	close_xml_element(HELP_EVENTS, 0, buffer);
+	close_xml_element(HELP_EVENTS, 0, buffer, MAX_BUFFER);
 	printf("%s", buffer);
 }
 
-static void xml_do_arch_specific_event_help(struct op_event const * event,
-					    char * buffer)
+static void xml_do_arch_specific_event_help(struct op_event const *event,
+					    char *buffer, size_t size)
 {
 	switch (cpu_type) {
 	case CPU_PPC64_CELL:
-		init_xml_int_attr(HELP_EVENT_GROUP, event->val / 100, buffer);
+		init_xml_int_attr(HELP_EVENT_GROUP, event->val / 100, buffer,
+				  size);
 		break;
 	default:
 		break;
@@ -60,34 +60,39 @@ void xml_help_for_event(struct op_event const * event)
 	uint i;
 	int nr_counters;
 	int has_nested = strcmp(event->unit->name, "zero");
-	char buffer[MAX_BUFFER];
 
 	buffer[0] = '\0';
-	open_xml_element(HELP_EVENT, 1, buffer);
-	init_xml_str_attr(HELP_EVENT_NAME, event->name, buffer);
-	xml_do_arch_specific_event_help(event, buffer);
-	init_xml_str_attr(HELP_EVENT_DESC, event->desc, buffer);
+	open_xml_element(HELP_EVENT, 1, buffer, MAX_BUFFER);
+	init_xml_str_attr(HELP_EVENT_NAME, event->name, buffer, MAX_BUFFER);
+	xml_do_arch_specific_event_help(event, buffer, MAX_BUFFER);
+	init_xml_str_attr(HELP_EVENT_DESC, event->desc, buffer, MAX_BUFFER);
 
 	nr_counters = op_get_nr_counters(cpu_type);
-	init_xml_int_attr(HELP_COUNTER_MASK, event->counter_mask, buffer);
-	init_xml_int_attr(HELP_MIN_COUNT, event->min_count, buffer);
+	init_xml_int_attr(HELP_COUNTER_MASK, event->counter_mask, buffer,
+			  MAX_BUFFER);
+	init_xml_int_attr(HELP_MIN_COUNT, event->min_count,
+			  buffer, MAX_BUFFER);
 
 	if (has_nested) {
-		close_xml_element(NONE, 1, buffer);
-		open_xml_element(HELP_UNIT_MASKS, 1, buffer);
-		init_xml_int_attr(HELP_DEFAULT_MASK, event->unit->default_mask, buffer);
-		close_xml_element(NONE, 1, buffer);
+		close_xml_element(NONE, 1, buffer, MAX_BUFFER);
+		open_xml_element(HELP_UNIT_MASKS, 1, buffer, MAX_BUFFER);
+		init_xml_int_attr(HELP_DEFAULT_MASK, event->unit->default_mask,
+				  buffer, MAX_BUFFER);
+		close_xml_element(NONE, 1, buffer, MAX_BUFFER);
 		for (i = 0; i < event->unit->num; i++) {
-			open_xml_element(HELP_UNIT_MASK, 1, buffer);
+			open_xml_element(HELP_UNIT_MASK, 1, buffer, MAX_BUFFER);
 			init_xml_int_attr(HELP_UNIT_MASK_VALUE,
-					  event->unit->um[i].value, buffer);
+					  event->unit->um[i].value,
+					  buffer, MAX_BUFFER);
 			init_xml_str_attr(HELP_UNIT_MASK_DESC,
-					  event->unit->um[i].desc, buffer);
-			close_xml_element(NONE, 0, buffer);
+					  event->unit->um[i].desc,
+					  buffer, MAX_BUFFER);
+			close_xml_element(NONE, 0, buffer, MAX_BUFFER);
 		}
-		close_xml_element(HELP_UNIT_MASKS, 0, buffer);
+		close_xml_element(HELP_UNIT_MASKS, 0, buffer, MAX_BUFFER);
 	}
-	close_xml_element(has_nested ? HELP_EVENT : NONE, has_nested, buffer);
+	close_xml_element(has_nested ? HELP_EVENT : NONE, has_nested,
+			  buffer, MAX_BUFFER);
 	printf("%s", buffer);
 }
 
diff --git a/libop/op_xml_out.c b/libop/op_xml_out.c
index d779c45..7cf0abb 100644
--- a/libop/op_xml_out.c
+++ b/libop/op_xml_out.c
@@ -91,143 +91,176 @@ char const * xml_tag_name(tag_t tag)
 }
 
 
-void open_xml_element(tag_t tag, int with_attrs, char * buffer)
+void open_xml_element(tag_t tag, int with_attrs, char *buffer, size_t max)
 {
-	char const * tag_name = xml_tag_name(tag);
-	unsigned int const max_len = strlen(tag_name) + 3;
-	char tmp_buf[MAX_BUF_LEN];
+	char *buf;
+	int size, ret;
 
-	if (max_len >= sizeof(tmp_buf))
-		fprintf(stderr,"Warning: open_xml_element: buffer overflow %d\n", max_len);
+	buffer[max - 1] = '\0';
+	size = strlen(buffer);
+	buf = &buffer[size];
+	size = max - 1 - size;
 
-	if (snprintf(tmp_buf, sizeof(tmp_buf), "<%s%s", tag_name,
-		(with_attrs ? " " : ">\n")) < 0) {
+	ret = snprintf(buf, size, "<%s%s", xml_tag_name(tag),
+		       (with_attrs ? " " : ">\n"));
+
+	if (ret < 0 || ret >= size) {
 		fprintf(stderr,"open_xml_element: snprintf failed\n");
 		exit(EXIT_FAILURE);
 	}
-	strncat(buffer, tmp_buf, sizeof(tmp_buf));
 }
 
 
-void close_xml_element(tag_t tag, int has_nested, char * buffer)
+void close_xml_element(tag_t tag, int has_nested, char *buffer, size_t max)
 {
-	char const * tag_name = xml_tag_name(tag);
-	unsigned int const max_len = strlen(tag_name) + 3;
-	char tmp_buf[MAX_BUF_LEN];
+	char *buf;
+	int size, ret;
 
-	if (max_len >= sizeof(tmp_buf))
-		fprintf(stderr,"Warning: close_xml_element: buffer overflow %d\n", max_len);
+	buffer[max - 1] = '\0';
+	size = strlen(buffer);
+	buf = &buffer[size];
+	size = max - 1 - size;
 
-	if (tag == NONE) {
-		if (snprintf(tmp_buf, sizeof(tmp_buf), "%s\n", (has_nested ? ">" : "/>")) < 0) {
-			fprintf(stderr, "close_xml_element: snprintf failed\n");
-			exit(EXIT_FAILURE);
-		}
-	} else {
-		if (snprintf(tmp_buf, sizeof(tmp_buf), "</%s>\n", tag_name) < 0) {
-			fprintf(stderr, "close_xml_element: snprintf failed\n");
-			exit(EXIT_FAILURE);
-		}
+	if (tag == NONE)
+		ret = snprintf(buf, size, "%s\n", (has_nested ? ">" : "/>"));
+	else
+		ret = snprintf(buf, size, "</%s>\n", xml_tag_name(tag));
+
+	if (ret < 0 || ret >= size) {
+		fprintf(stderr, "close_xml_element: snprintf failed\n");
+		exit(EXIT_FAILURE);
 	}
-	strncat(buffer, tmp_buf, sizeof(tmp_buf));
 }
 
 
-void init_xml_int_attr(tag_t attr, int value, char * buffer)
+void init_xml_int_attr(tag_t attr, int value, char *buffer, size_t max)
 {
-	char const * attr_name = xml_tag_name(attr);
-	char tmp_buf[MAX_BUF_LEN];
-	unsigned int const max_len = strlen(attr_name) + 50;
+	char *buf;
+	int size, ret;
 
-	if (max_len >= sizeof(tmp_buf)) {
-		fprintf(stderr,
-			"Warning: init_xml_int_attr: buffer overflow %d\n", max_len);
-	}
+	buffer[max - 1] = '\0';
+	size = strlen(buffer);
+	buf = &buffer[size];
+	size = max - 1 - size;
 
+	ret = snprintf(buf, size, " %s=\"%d\"", xml_tag_name(attr), value);
 
-	if (snprintf(tmp_buf, sizeof(tmp_buf), " %s=\"%d\"", attr_name, value) < 0) {
+	if (ret < 0 || ret >= size) {
 		fprintf(stderr,"init_xml_int_attr: snprintf failed\n");
 		exit(EXIT_FAILURE);
 	}
-	strncat(buffer, tmp_buf, sizeof(tmp_buf));
 }
 
 
-void init_xml_dbl_attr(tag_t attr, double value, char * buffer)
+void init_xml_dbl_attr(tag_t attr, double value, char *buffer, size_t max)
 {
-	char const * attr_name = xml_tag_name(attr);
-	unsigned int const max_len = strlen(attr_name) + 50;
-	char tmp_buf[MAX_BUF_LEN];
+	char *buf;
+	int size, ret;
+
+	buffer[max - 1] = '\0';
+	size = strlen(buffer);
+	buf = &buffer[size];
+	size = max - 1 - size;
 
-	if (max_len >= sizeof(tmp_buf))
-		fprintf(stderr, "Warning: init_xml_dbl_attr: buffer overflow %d\n", max_len);
+	ret = snprintf(buf, size, " %s=\"%.2f\"", xml_tag_name(attr), value);
 
-	if (snprintf(tmp_buf, sizeof(tmp_buf), " %s=\"%.2f\"", attr_name, value) < 0) {
+	if (ret < 0 || ret >= size) {
 		fprintf(stderr, "init_xml_dbl_attr: snprintf failed\n");
 		exit(EXIT_FAILURE);
 	}
-	strncat(buffer, tmp_buf, sizeof(tmp_buf));
 }
 
 
-static char * xml_quote(char const * str, char * quote_buf)
+static void xml_quote(char const *str, char *buffer, size_t max)
 {
-	int i;
-	int pos = 0;
-	int len = strlen(str);
+	char *buf;
+	char *quote;
+	char *pos = (char*)str;
+	size_t size;
+	int ret;
 
-	
-	quote_buf[pos++] = '"';
+	buffer[max - 1] = '\0';
+	size = strlen(buffer);
+	buf = &buffer[size];
+	size = max - 1 - size;
 
-	for (i = 0; i < len; i++) {
-		if (pos >= MAX_BUF_LEN - 10) {
-			fprintf(stderr,"quote_str: buffer overflow %d\n", pos);
-			exit(EXIT_FAILURE);
-		}
+	if (size < strlen(pos) + 2)
+		goto Error;
 
-		switch(str[i]) {
+	*buf = '"';
+	buf++;
+	size--;
+
+	while (*pos) {
+		switch(*pos) {
 		case '&':
-			strncpy(quote_buf + pos, "&amp;", 5);
-			pos += 5;
+			quote = "&amp;";
 			break;
 		case '<':
-			strncpy(quote_buf + pos, "&lt;", 4);
-			pos += 4;
+			quote = "&lt;";
 			break;
 		case '>':
-			strncpy(quote_buf + pos, "&gt;", 4);
-			pos += 4;
+                        quote = "&gt;";
 			break;
 		case '"':
-			strncpy(quote_buf + pos, "&quot;", 6);
-			pos += 6;
+			quote = "&quot;";
 			break;
 		default:
-			quote_buf[pos++] = str[i];
-			break;
+			*buf = *pos;
+			pos++;
+			buf++;
+			size--;
+			continue;
 		}
+
+		pos++;
+		ret = snprintf(buf, size, "%s", quote);
+		if (ret < 0 || ret >= (int)size)
+			goto Error;
+		buf += ret;
+		size -= ret;
+		if (size < strlen(pos))
+			goto Error;
 	}
 
-	quote_buf[pos++] = '"';
-	quote_buf[pos++] = '\0';
-	return quote_buf;
+	if (!size)
+		goto Error;
+
+	*buf = '"';
+	buf++;
+	*buf = '\0';
+
+	return;
+
+Error:
+	fprintf(stderr,"quote_str: buffer overflow\n");
+	exit(EXIT_FAILURE);
 }
 
 
-void init_xml_str_attr(tag_t attr, char const * str, char * buffer)
+void init_xml_str_attr(tag_t attr, char const *str, char *buffer, size_t max)
 {
-	char tmp_buf[MAX_BUF_LEN];
-	char quote_buf[MAX_BUF_LEN];
-	char const * attr_name = xml_tag_name(attr);
-	char const * quote_str = xml_quote(str, quote_buf);
-	const unsigned int max_len = strlen(attr_name) + strlen(quote_str) + 10;
+	char *buf;
+	int size, ret;
 
-	if (max_len >= sizeof(tmp_buf))
-		fprintf(stderr, "Warning: init_xml_str_attr: buffer overflow %d\n", max_len);
+	buffer[max - 1] = '\0';
+	size = strlen(buffer);
+	buf = &buffer[size];
+	size = max - 1 - size;
 
-	if (snprintf(tmp_buf, sizeof(tmp_buf), " %s=""%s""", attr_name, quote_str) < 0) {
-		fprintf(stderr,"init_xml_str_attr: snprintf failed\n");
-		exit(EXIT_FAILURE);
-	}
-	strncat(buffer, tmp_buf, sizeof(tmp_buf));
+	ret = snprintf(buf, size, " %s=", xml_tag_name(attr));
+	if (ret < 0 || ret >= size)
+		goto Error;
+
+	buf += ret;
+	size -= ret;
+
+	if (!size)
+		goto Error;
+
+	xml_quote(str, buf, size);
+	return;
+Error:
+	fprintf(stderr,"init_xml_str_attr: snprintf failed\n");
+	exit(EXIT_FAILURE);
 }
diff --git a/libop/op_xml_out.h b/libop/op_xml_out.h
index 52e8d8f..ae0fea5 100644
--- a/libop/op_xml_out.h
+++ b/libop/op_xml_out.h
@@ -59,11 +59,11 @@ typedef enum {
 	} tag_t;
 
 char const * xml_tag_name(tag_t tag);
-void open_xml_element(tag_t tag, int with_attrs, char * result);
-void close_xml_element(tag_t tag, int has_nested, char * result);
-void init_xml_int_attr(tag_t attr, int value, char * result);
-void init_xml_dbl_attr(tag_t attr, double value, char * result);
-void init_xml_str_attr(tag_t attr, char const * str, char * result);
+void open_xml_element(tag_t tag, int with_attrs, char *buffer, size_t size);
+void close_xml_element(tag_t tag, int has_nested, char *buffer, size_t size);
+void init_xml_int_attr(tag_t attr, int value, char *buffer, size_t size);
+void init_xml_dbl_attr(tag_t attr, double value, char *buffer, size_t size);
+void init_xml_str_attr(tag_t attr, char const *str, char *buffer, size_t size);
 
 #ifdef __cplusplus
 }
diff --git a/libutil++/xml_output.cpp b/libutil++/xml_output.cpp
index 0f0b189..4ac9a1b 100644
--- a/libutil++/xml_output.cpp
+++ b/libutil++/xml_output.cpp
@@ -16,7 +16,8 @@
 
 using namespace std;
 
-#define MAX_XML_BUF 1024
+#define MAX_XML_BUF 4096
+static char buf[MAX_XML_BUF];
 
 string tag_name(tag_t tag)
 {
@@ -29,10 +30,9 @@ string tag_name(tag_t tag)
 string open_element(tag_t tag, bool with_attrs)
 {
 	ostringstream out;
-	char buf[MAX_XML_BUF];
 
 	buf[0] = '\0';
-	open_xml_element(tag, with_attrs, buf);
+	open_xml_element(tag, with_attrs, buf, MAX_XML_BUF);
 	out << buf;
 	return out.str();
 }
@@ -41,10 +41,9 @@ string open_element(tag_t tag, bool with_attrs)
 string close_element(tag_t tag, bool has_nested)
 {
 	ostringstream out;
-	char buf[MAX_XML_BUF];
 
 	buf[0] = '\0';
-	close_xml_element(tag, has_nested, buf);
+	close_xml_element(tag, has_nested, buf, MAX_XML_BUF);
 	out << buf;
 	return out.str();
 }
@@ -53,10 +52,9 @@ string close_element(tag_t tag, bool has_nested)
 string init_attr(tag_t attr, size_t value)
 {
 	ostringstream out;
-	char buf[MAX_XML_BUF];
 
 	buf[0] = '\0';
-	init_xml_int_attr(attr, value, buf);
+	init_xml_int_attr(attr, value, buf, MAX_XML_BUF);
 	out << buf;
 	return out.str();
 }
@@ -65,10 +63,9 @@ string init_attr(tag_t attr, size_t value)
 string init_attr(tag_t attr, double value)
 {
 	ostringstream out;
-	char buf[MAX_XML_BUF];
 
 	buf[0] = '\0';
-	init_xml_dbl_attr(attr, value, buf);
+	init_xml_dbl_attr(attr, value, buf, MAX_XML_BUF);
 	out << buf;
 	return out.str();
 }
@@ -77,10 +74,9 @@ string init_attr(tag_t attr, double value)
 string init_attr(tag_t attr, string const & str)
 {
 	ostringstream out;
-	char buf[MAX_XML_BUF];
 
 	buf[0] = '\0';
-	init_xml_str_attr(attr, str.c_str(), buf);
+	init_xml_str_attr(attr, str.c_str(), buf, MAX_XML_BUF);
 	out << buf;
 	return out.str();
 }
-- 
1.6.4