/pidgin/main: 16ad7fd07d76: Use safer string building in msn_mes...

[Mark Doliner]

Changeset: 16ad7fd07d7651494df17e0db7bbe922ac753926
Author:	 Mark Doliner <mark at kingant.net>
Date:	 2014-02-05 23:52 -0800
Branch:	 default
URL: https://hg.pidgin.im/pidgin/main/rev/16ad7fd07d76

Description:

Use safer string building in msn_message_gen_payload

The old code used a series of calls to g_snprintf() to concatenate
strings onto a fixed sized buffer. This type of thing worries me.
I feel like it's easy to make a mistake and overwrite the end of the
buffer. The code also didn't handle messages larger than 8192 bytes.

The new code uses GStrings, which should be much safer and allows the
buffer to be realloc'ed to a larger size if needed.

I'm always tempted to use alloca for stuff like this. I really like
the idea of using stack space rather than memory allocation. But everytime
I read the alloca man page I get scared away from it. It sounds so
non-standard. And sounds like there's no way to fail gracefully if there's
not enough room on the stack--eek.

diffstat:

 libpurple/protocols/msn/msg.c |  67 +++++++++++++++++-------------------------
 1 files changed, 27 insertions(+), 40 deletions(-)

diffs (100 lines):

diff --git a/libpurple/protocols/msn/msg.c b/libpurple/protocols/msn/msg.c
--- a/libpurple/protocols/msn/msg.c
+++ b/libpurple/protocols/msn/msg.c
@@ -323,37 +323,30 @@ msn_message_new_from_cmd(MsnSession *ses
 char *
 msn_message_gen_payload(MsnMessage *msg, size_t *ret_size)
 {
+	GString *payload;
 	GList *l;
-	char *n, *base, *end;
-	int len;
-	size_t body_len = 0;
+	size_t body_len;
 	const void *body;
 
 	g_return_val_if_fail(msg != NULL, NULL);
 
-	len = MSN_BUF_LEN;
+	/* 8192 is a reasonable guess at a large enough buffer to avoid realloc */
+	payload = g_string_sized_new(8192);
 
-	base = n = end = g_malloc(len + 1);
-	end += len;
-
-	/* Standard header. */
-	if (msg->charset == NULL)
-	{
-		g_snprintf(n, len,
-				   "MIME-Version: 1.0\r\n"
-				   "Content-Type: %s\r\n",
-				   msg->content_type);
-	}
-	else
-	{
-		g_snprintf(n, len,
-				   "MIME-Version: 1.0\r\n"
-				   "Content-Type: %s; charset=%s\r\n",
-				   msg->content_type, msg->charset);
+	/* Standard header */
+	if (msg->charset == NULL) {
+		g_string_append_printf(payload,
+				"MIME-Version: 1.0\r\n"
+				"Content-Type: %s\r\n",
+				msg->content_type);
+	} else {
+		g_string_append_printf(payload,
+				"MIME-Version: 1.0\r\n"
+				"Content-Type: %s; charset=%s\r\n",
+				msg->content_type, msg->charset);
 	}
 
-	n += strlen(n);
-
+	/* Headers */
 	for (l = msg->header_list; l != NULL; l = l->next)
 	{
 		const char *key;
@@ -362,31 +355,25 @@ msn_message_gen_payload(MsnMessage *msg,
 		key = l->data;
 		value = msn_message_get_header_value(msg, key);
 
-		g_snprintf(n, end - n, "%s: %s\r\n", key, value);
-		n += strlen(n);
+		g_string_append_printf(payload, "%s: %s\r\n", key, value);
 	}
 
-	if ((end - n) > 2)
-		n += g_strlcpy(n, "\r\n", end - n);
+	/* End of headers */
+	g_string_append(payload, "\r\n");
 
+	/* Body */
 	body = msn_message_get_bin_data(msg, &body_len);
-
-	if (body != NULL && (size_t)(end - n) > body_len)
-	{
-		memcpy(n, body, body_len);
-		n += body_len;
-		*n = '\0';
+	if (body != NULL) {
+		g_string_append_len(payload, body, body_len);
 	}
 
-	if (ret_size != NULL)
-	{
-		*ret_size = n - base;
-
-		if (*ret_size > 1664)
-			*ret_size = 1664;
+	if (ret_size != NULL) {
+		/* Use MIN to truncate the payload to 1664 bytes? Why do we do this?
+		   It seems like it will lead to brokenness. */
+		*ret_size = MIN(payload->len, 1664);
 	}
 
-	return base;
+	return g_string_free(payload, FALSE);
 }
 
 void