(svn r14540) -Codechange: introduce [v]seprintf which are like [v]snprintf but do return the number of characters written instead of the number of characters that would be written; as size_t is unsigned substraction can cause integer underflows quite quickly.

src/blitter/factory.hpp

@@ -119,13 +119,13 @@ public:
 
 	static char *GetBlittersInfo(char *p, const char *last)
 	{
-		p += snprintf(p, last - p, "List of blitters:\n");
+		p += seprintf(p, last, "List of blitters:\n");
 		Blitters::iterator it = GetBlitters().begin();
 		for (; it != GetBlitters().end(); it++) {
 			BlitterFactoryBase *b = (*it).second;
-			p += snprintf(p, last - p, "%18s: %s\n", b->name, b->GetDescription());
+			p += seprintf(p, last, "%18s: %s\n", b->name, b->GetDescription());
 		}
-		p += snprintf(p, last - p, "\n");
+		p += seprintf(p, last, "\n");
 
 		return p;
 	}

src/driver.cpp

@@ -168,7 +168,7 @@ void DriverFactoryBase::RegisterDriver(const char *name, Driver::Type type, int
 char *DriverFactoryBase::GetDriversInfo(char *p, const char *last)
 {
 	for (Driver::Type type = Driver::DT_BEGIN; type != Driver::DT_END; type++) {
-		p += snprintf(p, last - p, "List of %s drivers:\n", GetDriverTypeName(type));
+		p += seprintf(p, last, "List of %s drivers:\n", GetDriverTypeName(type));
 
 		for (int priority = 10; priority >= 0; priority--) {
 			Drivers::iterator it = GetDrivers().begin();
@@ -176,11 +176,11 @@ char *DriverFactoryBase::GetDriversInfo(char *p, const char *last)
 				DriverFactoryBase *d = (*it).second;
 				if (d->type != type) continue;
 				if (d->priority != priority) continue;
-				p += snprintf(p, last - p, "%18s: %s\n", d->name, d->GetDescription());
+				p += seprintf(p, last, "%18s: %s\n", d->name, d->GetDescription());
 			}
 		}
 
-		p += snprintf(p, last - p, "\n");
+		p += seprintf(p, last, "\n");
 	}
 
 	return p;

src/gfxinit.cpp

@@ -240,10 +240,11 @@ void CheckExternalFiles()
 	char error_msg[ERROR_MESSAGE_LENGTH * (MAX_GFT + 1)];
 	error_msg[0] = '\0';
 	char *add_pos = error_msg;
+	const char *last = lastof(error_msg);
 
 	for (uint i = 0; i < lengthof(_used_graphics_set->files); i++) {
 		if (!FileMD5(_used_graphics_set->files[i])) {
-			add_pos += snprintf(add_pos, ERROR_MESSAGE_LENGTH, "Your '%s' file is corrupted or missing! %s\n", _used_graphics_set->files[i].filename, _used_graphics_set->files[i].missing_warning);
+			add_pos += seprintf(add_pos, last, "Your '%s' file is corrupted or missing! %s\n", _used_graphics_set->files[i].filename, _used_graphics_set->files[i].missing_warning);
 		}
 	}
 
@@ -253,7 +254,7 @@ void CheckExternalFiles()
 	}
 
 	if (!sound) {
-		add_pos += snprintf(add_pos, ERROR_MESSAGE_LENGTH, "Your 'sample.cat' file is corrupted or missing! You can find 'sample.cat' on your Transport Tycoon Deluxe CD-ROM.\n");
+		add_pos += seprintf(add_pos, last, "Your 'sample.cat' file is corrupted or missing! You can find 'sample.cat' on your Transport Tycoon Deluxe CD-ROM.\n");
 	}
 
 	if (add_pos != error_msg) ShowInfoF(error_msg);
@@ -526,19 +527,19 @@ bool SetGraphicsSet(const char *name)
  */
 char *GetGraphicsSetsList(char *p, const char *last)
 {
-	p += snprintf(p, last - p, "List of graphics sets:\n");
+	p += seprintf(p, last, "List of graphics sets:\n");
 	for (const GraphicsSet *g = _available_graphics_sets; g != NULL; g = g->next) {
 		if (g->found_grfs <= 1) continue;
 
-		p += snprintf(p, last - p, "%18s: %s", g->name, g->description);
+		p += seprintf(p, last, "%18s: %s", g->name, g->description);
 		int difference = MAX_GFT - g->found_grfs;
 		if (difference != 0) {
-			p += snprintf(p, last - p, " (missing %i file%s)\n", difference, difference == 1 ? "" : "s");
+			p += seprintf(p, last, " (missing %i file%s)\n", difference, difference == 1 ? "" : "s");
 		} else {
-			p += snprintf(p, last - p, "\n");
+			p += seprintf(p, last, "\n");
 		}
 	}
-	p += snprintf(p, last - p, "\n");
+	p += seprintf(p, last, "\n");
 
 	return p;
 }

src/newgrf_config.cpp

@@ -479,7 +479,7 @@ char *GRFBuildParamList(char *dst, const GRFConfig *c, const char *last)
 
 	for (i = 0; i < c->num_params; i++) {
 		if (i > 0) dst = strecpy(dst, " ", last);
-		dst += snprintf(dst, last - dst, "%d", c->param[i]);
+		dst += seprintf(dst, last, "%d", c->param[i]);
 	}
 	return dst;
 }

src/openttd.cpp

@@ -166,7 +166,7 @@ static void ShowHelp()
 	char buf[4096];
 	char *p = buf;
 
-	p += snprintf(p, lengthof(buf), "OpenTTD %s\n", _openttd_revision);
+	p += seprintf(p, lastof(buf), "OpenTTD %s\n", _openttd_revision);
 	p = strecpy(p,
 		"\n"
 		"\n"

src/settings.cpp

@@ -211,10 +211,11 @@ static bool load_intlist(const char *str, void *array, int nelems, VarType type)
 /** Convert an integer-array (intlist) to a string representation. Each value
  * is seperated by a comma or a space character
  * @param buf output buffer where the string-representation will be stored
+ * @param last last item to write to in the output buffer
  * @param array pointer to the integer-arrays that is read from
  * @param nelems the number of elements the array holds.
  * @param type the type of elements the array holds (eg INT8, UINT16, etc.) */
-static void make_intlist(char *buf, const void *array, int nelems, VarType type)
+static void make_intlist(char *buf, const char *last, const void *array, int nelems, VarType type)
 {
 	int i, v = 0;
 	const byte *p = (const byte*)array;
@@ -230,15 +231,16 @@ static void make_intlist(char *buf, const void *array, int nelems, VarType type)
 		case SLE_VAR_U32: v = *(uint32*)p; p += 4; break;
 		default: NOT_REACHED();
 		}
-		buf += sprintf(buf, (i == 0) ? "%d" : ",%d", v);
+		buf += seprintf(buf, last, (i == 0) ? "%d" : ",%d", v);
 	}
 }
 
 /** Convert a ONEofMANY structure to a string representation.
  * @param buf output buffer where the string-representation will be stored
+ * @param last last item to write to in the output buffer
  * @param many the full-domain string of possible values
  * @param id the value of the variable and whose string-representation must be found */
-static void make_oneofmany(char *buf, const char *many, int id)
+static void make_oneofmany(char *buf, const char *last, const char *many, int id)
 {
 	int orig_id = id;
 
@@ -246,7 +248,7 @@ static void make_oneofmany(char *buf, const char *many, int id)
 	while (--id >= 0) {
 		for (; *many != '|'; many++) {
 			if (*many == '\0') { // not found
-				sprintf(buf, "%d", orig_id);
+				seprintf(buf, last, "%d", orig_id);
 				return;
 			}
 		}
@@ -254,16 +256,17 @@ static void make_oneofmany(char *buf, const char *many, int id)
 	}
 
 	/* copy string until next item (|) or the end of the list if this is the last one */
-	while (*many != '\0' && *many != '|') *buf++ = *many++;
+	while (*many != '\0' && *many != '|' && buf < last) *buf++ = *many++;
 	*buf = '\0';
 }
 
 /** Convert a MANYofMANY structure to a string representation.
  * @param buf output buffer where the string-representation will be stored
+ * @param last last item to write to in the output buffer
  * @param many the full-domain string of possible values
  * @param x the value of the variable and whose string-representation must
  *        be found in the bitmasked many string */
-static void make_manyofmany(char *buf, const char *many, uint32 x)
+static void make_manyofmany(char *buf, const char *last, const char *many, uint32 x)
 {
 	const char *start;
 	int i = 0;
@@ -274,10 +277,10 @@ static void make_manyofmany(char *buf, const char *many, uint32 x)
 		while (*many != 0 && *many != '|') many++; // advance to the next element
 
 		if (HasBit(x, 0)) { // item found, copy it
-			if (!init) *buf++ = '|';
+			if (!init) buf += seprintf(buf, last, "|");
 			init = false;
 			if (start == many) {
-				buf += sprintf(buf, "%d", i);
+				buf += seprintf(buf, last, "%d", i);
 			} else {
 				memcpy(buf, start, many - start);
 				buf += many - start;
@@ -556,9 +559,9 @@ static void ini_save_settings(IniFile *ini, const SettingDesc *sd, const char *g
 
 			switch (sdb->cmd) {
 			case SDT_BOOLX:      strcpy(buf, (i != 0) ? "true" : "false"); break;
-			case SDT_NUMX:       sprintf(buf, IsSignedVarMemType(sld->conv) ? "%d" : "%u", i); break;
+			case SDT_NUMX:       seprintf(buf, lastof(buf), IsSignedVarMemType(sld->conv) ? "%d" : "%u", i); break;
-			case SDT_ONEOFMANY:  make_oneofmany(buf, sdb->many, i); break;
+			case SDT_ONEOFMANY:  make_oneofmany(buf, lastof(buf), sdb->many, i); break;
-			case SDT_MANYOFMANY: make_manyofmany(buf, sdb->many, i); break;
+			case SDT_MANYOFMANY: make_manyofmany(buf, lastof(buf), sdb->many, i); break;
 			default: NOT_REACHED();
 			}
 		} break;
@@ -566,16 +569,16 @@ static void ini_save_settings(IniFile *ini, const SettingDesc *sd, const char *g
 		case SDT_STRING:
 			switch (GetVarMemType(sld->conv)) {
 			case SLE_VAR_STRB: strcpy(buf, (char*)ptr); break;
-			case SLE_VAR_STRBQ:sprintf(buf, "\"%s\"", (char*)ptr); break;
+			case SLE_VAR_STRBQ:seprintf(buf, lastof(buf), "\"%s\"", (char*)ptr); break;
 			case SLE_VAR_STR:  strcpy(buf, *(char**)ptr); break;
-			case SLE_VAR_STRQ: sprintf(buf, "\"%s\"", *(char**)ptr); break;
+			case SLE_VAR_STRQ: seprintf(buf, "\"%s\"", lastof(buf), *(char**)ptr); break;
-			case SLE_VAR_CHAR: sprintf(buf, "\"%c\"", *(char*)ptr); break;
+			case SLE_VAR_CHAR: seprintf(buf, "\"%c\"", lastof(buf), *(char*)ptr); break;
 			default: NOT_REACHED();
 			}
 			break;
 
 		case SDT_INTLIST:
-			make_intlist(buf, ptr, sld->length, GetVarMemType(sld->conv));
+			make_intlist(buf, lastof(buf), ptr, sld->length, GetVarMemType(sld->conv));
 			break;
 		default: NOT_REACHED();
 		}

src/string.cpp

@@ -6,6 +6,7 @@
 #include "openttd.h"
 #include "debug.h"
 #include "core/alloc_func.hpp"
+#include "core/math_func.hpp"
 #include "string_func.h"
 
 #include "table/control_codes.h"
@@ -59,17 +60,16 @@ char* strecpy(char* dst, const char* src, const char* last)
 }
 
 
-char* CDECL str_fmt(const char* str, ...)
+char *CDECL str_fmt(const char *str, ...)
 {
 	char buf[4096];
 	va_list va;
-	int len;
 
 	va_start(va, str);
-	len = vsnprintf(buf, lengthof(buf), str, va);
+	int len = vseprintf(buf, lastof(buf), str, va);
 	va_end(va);
-	char* p = MallocT<char>(len + 1);
+	char *p = MallocT<char>(len + 1);
-	if (p != NULL) memcpy(p, buf, len + 1);
+	memcpy(p, buf, len + 1);
 	return p;
 }
 
@@ -185,6 +185,43 @@ int CDECL vsnprintf(char *str, size_t size, const char *format, va_list ap)
 
 #endif /* WIN32 */
 
+/**
+ * Safer implementation of snprintf; same as snprintf except:
+ * - last instead of size, i.e. replace sizeof with lastof.
+ * - return gives the amount of characters added, not what it would add.
+ * @param str    buffer to write to up to last
+ * @param last   last character we may write to
+ * @param format the formatting (see snprintf)
+ * @return the number of added characters
+ */
+int CDECL seprintf(char *str, const char *last, const char *format, ...)
+{
+	va_list ap;
+
+	va_start(ap, format);
+	int ret = vseprintf(str, last, format, ap);
+	va_end(ap);
+	return ret;
+}
+
+/**
+ * Safer implementation of vsnprintf; same as vsnprintf except:
+ * - last instead of size, i.e. replace sizeof with lastof.
+ * - return gives the amount of characters added, not what it would add.
+ * @param str    buffer to write to up to last
+ * @param last   last character we may write to
+ * @param format the formatting (see snprintf)
+ * @param ap     the list of arguments for the format
+ * @return the number of added characters
+ */
+int CDECL vseprintf(char *str, const char *last, const char *format, va_list ap)
+{
+	if (str >= last) return 0;
+	size_t size = last - str;
+	return min((int)size, vsnprintf(str, size, format, ap));
+}
+
+
 
 /** Convert the md5sum to a hexadecimal string representation
  * @param buf buffer to put the md5sum into
@@ -196,8 +233,7 @@ char *md5sumToString(char *buf, const char *last, const uint8 md5sum[16])
 	char *p = buf;
 
 	for (uint i = 0; i < 16; i++) {
-		p += snprintf(p, last + 1 - p, "%02X", md5sum[i]);
+		p += seprintf(p, last, "%02X", md5sum[i]);
-		if (p >= last) break;
 	}
 
 	return p;

src/string_func.h

@@ -1,6 +1,19 @@
 /* $Id$ */
 
-/** @file string_func.h Functions related to low-level strings. */
+/** @file string_func.h Functions related to low-level strings.
+ *
+ * @note Be aware of "dangerous" string functions; string functions that
+ * have behaviour that could easily cause buffer overruns and such:
+ * - strncpy: does not '\0' terminate when input string is longer than
+ *   the size of the output string. Use strecpy instead.
+ * - [v]snprintf: returns the length of the string as it would be written
+ *   when the output is large enough, so it can be more than the size of
+ *   the buffer and than can underflow size_t (uint-ish) which makes all
+ *   subsequent snprintf alikes write outside of the buffer. Use
+ *   [v]seprintf instead; it will return the number of bytes actually
+ *   added so no [v]seprintf will cause outside of bounds writes.
+ * - [v]sprintf: does not bounds checking: use [v]seprintf instead.
+ */
 
 #ifndef STRING_FUNC_H
 #define STRING_FUNC_H
@@ -28,6 +41,9 @@ void ttd_strlcpy(char *dst, const char *src, size_t size);
 char *strecat(char *dst, const char *src, const char *last);
 char *strecpy(char *dst, const char *src, const char *last);
 
+int CDECL seprintf(char *str, const char *last, const char *format, ...);
+int CDECL vseprintf(char *str, const char *last, const char *format, va_list ap);
+
 char *CDECL str_fmt(const char *str, ...);
 
 /** Scans the string for valid characters and if it finds invalid ones,

src/strings.cpp

@@ -271,7 +271,7 @@ static char *FormatNoCommaNumber(char *buff, int64 number, const char *last)
 
 static char *FormatHexNumber(char *buff, int64 number, const char *last)
 {
-	return buff + snprintf(buff, last - buff, "0x%x", (uint32)number);
+	return buff + seprintf(buff, last, "0x%x", (uint32)number);
 }
 
 static char *FormatYmdString(char *buff, Date date, const char* last)
@@ -1198,8 +1198,8 @@ static char *GetSpecialNameString(char *buff, int ind, const int64 *argv, const
 	/* resolution size? */
 	if (IsInsideMM(ind, (SPECSTR_RESOLUTION_START - 0x70E4), (SPECSTR_RESOLUTION_END - 0x70E4) + 1)) {
 		int i = ind - (SPECSTR_RESOLUTION_START - 0x70E4);
-		buff += snprintf(
+		buff += seprintf(
-			buff, last - buff + 1, "%dx%d", _resolutions[i].width, _resolutions[i].height
+			buff, last, "%dx%d", _resolutions[i].width, _resolutions[i].height
 		);
 		return buff;
 	}