On Sun, Nov 15, 2009 at 10:12 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Jeff King <peff@xxxxxxxx> writes: > >> On Sun, Nov 15, 2009 at 05:36:54PM -0800, Shawn O. Pearce wrote: >> ... >>> Shouldn't this instead be: >>> >>> diff --git a/http-backend.c b/http-backend.c >>> index 9021266..16ec635 100644 >>> --- a/http-backend.c >>> +++ b/http-backend.c >>> @@ -626,7 +626,7 @@ int main(int argc, char **argv) >>> } >>> >>> cmd = c; >>> - cmd_arg = xmalloc(n); >>> + cmd_arg = xmalloc(n + 1); >>> strncpy(cmd_arg, dir + out[0].rm_so + 1, n); >>> cmd_arg[n] = '\0'; >>> dir[out[0].rm_so] = 0; >>> >>> The cmd_arg string was simply allocated too small. Your fix is >>> terminating the string one character too short which would cause >>> get_loose_object and get_pack_file to break. >> >> Actually, from my reading, I think his fix is right, because you trim >> the first character during the strncpy (using "out[0].rm_so + 1"). > > Your regexps all start with leading "/", and rm_so+1 points at the > character after the slash; the intention being that you would copy > the rest of the matched sequence without the leading "/". > > So allocating n = rm_eo - rm_so is Ok. It counts the space for > terminating NUL. But copying "up to n bytes" using strncpy(), only to NUL > terminate immediately later, is dubious. You would want to copy only n-1 > bytes. I.e. > > n = out[0].rm_eo - out[0].rm_so; /* allocation */ > ... validate and fail invalid method ... > cmd_arg = xmalloc(n); > memcpy(cmd_arg, dir + out[0].rm_so + 1, n-1); > cmd_arg[n-1] = '\0'; > I think the strncpy( , ,n) would not harm anything because we won't overflow dir because it's NUL terminated in getdir(), and the '\0' shouldn't match the regex. But I agree that strncpy( , , n-1) is better and memcpy( , , n-1) is better still. Better eyes than mine have now looked at this and see different things each time. I wonder if some parts could be made a little less subtle (perhaps along with the dir[out[0].rm_so] = 0;)? Thanks, Tarmigan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html