Re: Buffer overflows

[Timo Sirainen <tss@iki.fi>]

[-- Attachment #1.1: Type: text/plain, Size: 1889 bytes --]

On Thu, 2007-08-30 at 21:09 -0700, Linus Torvalds wrote:
> 
> On Fri, 31 Aug 2007, Timo Sirainen wrote:
> > > 
> > > Perhaps because your patch was using a totally nonstandard and slow
> > > interface, and had nasty string declaration issues, as people even pointed
> > > out to you.
> > 
> > Slow?
> 
> Having a string library, and then implementing "str_append()" with a 
> strlen() sounds pretty disgusting to me. 
> 
> Gcc could have optimized the strlen() away for constant string arguments, 
> but since you made the thing out-of-line, it can't do that any more.
> 
> So yes, I bet there are faster string libraries out there.

Oh, well that's easy to fix. But I don't think the speed matters much in
string manipulation, it's usually not done in performance critical
paths.

> > The code should be easy to verify to be secure, and with some kind of a safe
> > string API it's a lot easier than trying to figure out corner cases where
> > strcpy() calls break.
> 
> I actually looked at the patches, and the "stringbuf()" thing was just too 
> ugly to live. It was also nonportable, in that you use the reserved 
> namespace (which we do extensively in the kernel, but that's an 
> "embdedded" application that doesn't use system header files).
> 
> So the API was anything but "safe".

Some of those were fixed in the patch I sent at the beginning of this
thread. But since you asked, attached is yet another version of it. As
far as I know it's fully C99 compatible. Would be C89 but there's that
va_copy() call.

I guess it could still be optimized more, but at least strlen() is now
in a macro. :)

Or if you don't like the STR_STATIC() macro, another way would be:

#define STR_STATIC_INIT(buf) { buf, sizeof(buf), 0, }

char static_buf[1024];
struct string str = STR_STATIC_INIT(static_buf);

str_append(&str, "hello world");


[-- Attachment #1.2: git-strings.diff --]
[-- Type: text/x-patch, Size: 3063 bytes --]

--- /dev/null	2007-07-12 03:40:41.165212167 +0300
+++ str.h	2007-08-31 07:35:04.000000000 +0300
@@ -0,0 +1,33 @@
+#ifndef STR_H
+#define STR_H
+
+struct string {
+	char *buf;
+
+	unsigned int alloc_size;
+	unsigned int len;
+
+	unsigned int malloced:1;
+	unsigned int overflowed:1;
+};
+
+#define STR_STATIC(name, size) \
+	struct { \
+	  struct string str; \
+	  char string_buf[(size) + 1]; \
+	} name = { { (name).string_buf, sizeof((name).string_buf), 0, } }
+
+extern struct string *str_alloc(unsigned int initial_size);
+extern void str_free(struct string **str);
+
+extern void str_append_n(struct string *str, const char *cstr,
+			 unsigned int len);
+extern void str_printfa(struct string *str, const char *fmt, ...)
+	__attribute__((format (printf, 2, 3)));
+extern void str_truncate(struct string *str, unsigned int len);
+
+#define str_append(str, cstr) str_append_n(str, cstr, strlen(cstr))
+#define str_c(str) ((str)->buf)
+#define str_len(str) ((str)->len)
+
+#endif
--- /dev/null	2007-07-12 03:40:41.165212167 +0300
+++ str.c	2007-08-31 07:36:56.000000000 +0300
@@ -0,0 +1,85 @@
+#include "str.h"
+#include "git-compat-util.h"
+
+struct string *str_alloc(unsigned int initial_size)
+{
+	struct string *str;
+
+	str = xcalloc(sizeof(*str), 1);
+	str->alloc_size = initial_size + 1;
+	str->buf = xmalloc(str->alloc_size);
+	str->buf[0] = '\0';
+	return str;
+}
+
+void str_free(struct string **_str)
+{
+	struct string *str = *_str;
+
+	if (str->malloced)
+		free(str->buf);
+	str->buf = NULL;
+	free(str);
+
+	*_str = NULL;
+}
+
+static void str_grow_if_needed(struct string *str, unsigned int *len)
+{
+	unsigned int avail = str->alloc_size - str->len;
+
+	if (*len >= avail) {
+		if (!str->malloced) {
+			/* static buffer size */
+			*len = avail;
+			str->overflowed = 1;
+		} else {
+			str->alloc_size = (str->len + *len) * 2;
+			str->buf = xrealloc(str->buf, str->alloc_size);
+		}
+	}
+}
+
+void str_append_n(struct string *str, const char *cstr, unsigned int len)
+{
+	str_grow_if_needed(str, &len);
+	memcpy(str->buf + str->len, cstr, len);
+	str->len += len;
+	str->buf[str->len] = '\0';
+}
+
+void str_printfa(struct string *str, const char *fmt, ...)
+{
+	unsigned int len, avail = str->alloc_size - str->len;
+	va_list va, va2;
+	int ret;
+
+	va_start(va, fmt);
+	va_copy(va2, va);
+	ret = vsnprintf(str->buf + str->len, avail, fmt, va);
+	assert(ret >= 0);
+
+	if ((unsigned int)ret >= avail) {
+		if (!str->malloced) {
+			str->overflowed = 1;
+			ret = avail == 0 ? 0 : avail-1;
+		} else {
+			len = ret;
+			str_grow_if_needed(str, &len);
+			avail = str->alloc_size - str->len;
+
+			ret = vsnprintf(str->buf + str->len, avail, fmt, va2);
+			assert(ret >= 0 && (unsigned int)ret < avail);
+		}
+	}
+	str->len += ret;
+	va_end(va);
+}
+
+void str_truncate(struct string *str, unsigned int len)
+{
+	if (len >= str->alloc_size)
+		len = str->alloc_size - 1;
+	str->len = len;
+	str->buf[len] = '\0';
+}

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]