On Sat, 2007-05-19 at 22:31 +0300, Timo Sirainen wrote: > > or Git? > > It seems a bit kludgy with all of its different commands and scripts. > Also I don't really like its code. It's using standard C functions for > string manipulations and in general it's using a lot with fixed size > buffers. If it wasn't written by kernel developers, I'd say it's most > likely full of buffer overflows. But since it is (was?), perhaps there > are only a few. I don't want to risk it.
Just out of curiosity I checked it. There are even basic string manipulation errors in their code: ~/src/git-1.5.1% grep +=.*snprintf *.c builtin-grep.c: len += snprintf(argptr, sizeof(randarg)-len, builtin-grep.c: len += snprintf(argptr, sizeof(randarg)-len, builtin-grep.c: len += snprintf(argptr, sizeof(randarg)-len, commit.c: i += snprintf(parents + i, sizeof(parents) - i - 1, " %s", commit.c: i += snprintf(parents + i, sizeof(parents) - i - 1, " %s", diff.c: len += snprintf(msg + len, sizeof(msg) - len, diff.c: len += snprintf(msg + len, sizeof(msg) - len, diff.c: len += snprintf(msg + len, sizeof(msg) - len, diff.c: len += snprintf(msg + len, sizeof(msg) - len, diff.c: len += snprintf(msg + len, sizeof(msg) - len, diff.c: len += snprintf(msg + len, sizeof(msg) - len, "\n"); path.c: len += vsnprintf(pathname + len, PATH_MAX - len, fmt, args); Every single one of those is wrong. Linux kernel's snprintf() handles code like this safely, but libc doesn't. Possibly other snprintf() uses are wrong as well. strncpy() then: ~/src/git-1.5.1% grep strncpy *.c builtin-log.c: strncpy(ref_message_id, message_id, builtin-shortlog.c: strncpy(name, realname, maxlen); diff-lib.c: strncpy(buffer1 + len1, p1.items[i1++].path, diff-lib.c: strncpy(buffer2 + len2, p2.items[i2++].path, fast-import.c: strncpy(c->str_dat, s, len); fast-import.c: strncpy(ident, buf, name_len); interpolate.c: strncpy(dest, value, valuelen); trace.c: strncpy(trace_str, format_str, format_len); trace.c: strncpy(trace_str + format_len, argv_str, argv_len); correct, wrong, wrong, wrong, correct, correct The last 3 are just stupid. They should be memcpy()s to make it clear what they intend to do. I don't even want to start verifying the correctness of sprintf() and strcpy() calls. Wonder if I should have Cc'd this to git list. Oh well, someone else can if they care.
signature.asc
Description: This is a digitally signed message part
