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.