From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 27 Sep 2012 22:48:06 GMT
Subject: Re: [PATCH 3/8] longest_ancestor_length(): use string_list_split()
Message-ID: <7vr4pnnkux.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1348688090-13648-4-git-send-email-mhagger@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

> -	for (colon = ceil = prefix_list; *colon; ceil = colon+1) {
> -		for (colon = ceil; *colon && *colon != PATH_SEP; colon++);
> -		len = colon - ceil;
> +	string_list_split(&prefixes, prefix_list, PATH_SEP, -1);
> +
> +	for (i = 0; i < prefixes.nr; i++) {
> +		const char *ceil = prefixes.items[i].string;
> +		int len = strlen(ceil);
> +

Much nicer than the yucky original ;-)

>  		if (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))
>  			continue;
> -		strlcpy(buf, ceil, len+1);
> +		memcpy(buf, ceil, len+1);
>  		if (normalize_path_copy(buf, buf) < 0)
>  			continue;

Why do you need this memcpy in the first place?  Isn't ceil already
a NUL terminated string unlike the original code that points into a
part of the prefix_list string?  IOW, why not

	normalize_path_copy(buf, ceil);

or something?

Can normalize_path_copy() overflow buf[PATH_MAX+1] here (before or
after this patch)?