link_alt_odb_entry: handle normalize_path errors

When we add a new alternate to the list, we try to normalize out any redundant "..", etc. However, we do not look at the return value of normalize_path_copy(), and will happily continue with a path that could not be normalized. Worse, the normalizing process is done in-place, so we are left with whatever half-finished working state the normalizing function was in. Fortunately, this cannot cause us to read past the end of our buffer, as that working state will always leave the NUL from the original path in place. And we do tend to notice problems when we check is_directory() on the path. But you can see the nonsense that we feed to is_directory with an entry like: this/../../is/../../way/../../too/../../deep/../../to/../../resolve in your objects/info/alternates, which yields: error: object directory /to/e/deep/too/way//ects/this/../../is/../../way/../../too/../../deep/../../to/../../resolve does not exist; check .git/objects/info/alternates. We can easily fix this just by checking the return value. But that makes it hard to generate a good error message, since we're normalizing in-place and our input value has been overwritten by cruft. Instead, let's provide a strbuf helper that does an in-place normalize, but restores the original contents on error. This uses a second buffer under the hood, which is slightly less efficient, but this is not a performance-critical code path. The strbuf helper can also properly set the "len" parameter of the strbuf before returning. Just doing: normalize_path_copy(buf.buf, buf.buf); will shorten the string, but leave buf.len at the original length. That may be confusing to later code which uses the strbuf. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Oct 3, 2016 at 16:34 UTC 670c359da357639f9f9a814ed646b4d854ec5d55
3 files changed +37 -2
sha1_file.c
+9 -2
@@ -263,7 +263,12 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,
263 }
264 strbuf_addstr(&pathbuf, entry);
265
266 - normalize_path_copy(pathbuf.buf, pathbuf.buf);
266 + if (strbuf_normalize_path(&pathbuf) < 0) {
267 + error("unable to normalize alternate object path: %s",
268 + pathbuf.buf);
269 + strbuf_release(&pathbuf);
270 + return -1;
271 + }
272
273 pfxlen = strlen(pathbuf.buf);
274
@@ -335,7 +340,9 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,
340 }
341
342 strbuf_add_absolute_path(&objdirbuf, get_object_directory());
338 - normalize_path_copy(objdirbuf.buf, objdirbuf.buf);
343 + if (strbuf_normalize_path(&objdirbuf) < 0)
344 + die("unable to normalize object directory: %s",
345 + objdirbuf.buf);
346
347 alt_copy = xmemdupz(alt, len);
348 string_list_split_in_place(&entries, alt_copy, sep, -1);
strbuf.c
+20
@@ -870,3 +870,23 @@ void strbuf_stripspace(struct strbuf *sb, int skip_comments)
870
871 strbuf_setlen(sb, j);
872 }
873 +
874 +int strbuf_normalize_path(struct strbuf *src)
875 +{
876 + struct strbuf dst = STRBUF_INIT;
877 +
878 + strbuf_grow(&dst, src->len);
879 + if (normalize_path_copy(dst.buf, src->buf) < 0) {
880 + strbuf_release(&dst);
881 + return -1;
882 + }
883 +
884 + /*
885 + * normalize_path does not tell us the new length, so we have to
886 + * compute it by looking for the new NUL it placed
887 + */
888 + strbuf_setlen(&dst, strlen(dst.buf));
889 + strbuf_swap(src, &dst);
890 + strbuf_release(&dst);
891 + return 0;
892 +}
strbuf.h
+8
@@ -443,6 +443,14 @@ extern int strbuf_getcwd(struct strbuf *sb);
443 */
444 extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);
445
446 +
447 +/**
448 + * Normalize in-place the path contained in the strbuf. See
449 + * normalize_path_copy() for details. If an error occurs, the contents of "sb"
450 + * are left untouched, and -1 is returned.
451 + */
452 +extern int strbuf_normalize_path(struct strbuf *sb);
453 +
454 /**
455 * Strip whitespace from a buffer. The second parameter controls if
456 * comments are considered contents to be removed or not.