Michael Haggerty [off-list ref] 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)?
On 09/28/2012 12:48 AM, Junio C Hamano wrote:
Michael Haggerty [off-list ref] writes:
quoted
- 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 your winky-smiley implies irony, then I would like to object. Even
though the original is not difficult to understand, it is more difficult
to review than the new version because one has to think about off-by-one
errors etc. The new version has a bit more boilerplate, but a quick
read suffices both to understand it and to see that it is correct.
Though of course I admit that the improvement is small.
But the main point of this change is to move towards using more testable
parts, so it is a step forward regardless of whether it is more readable.
quoted
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?
Good point. I will fix this in v2.
Can normalize_path_copy() overflow buf[PATH_MAX+1] here (before or
after this patch)?
normalize_path_copy() can only shrink paths, not grow them. So the
length check on ceil guarantees that the result of normalize_path_copy()
will fit in buf.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/