From b11b7e13f48ee283738e21fea9f775cd40ca0562 Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Wed, 17 Dec 2008 09:36:40 -0800 Subject: [PATCH 1/8] Add generic 'strbuf_readlink()' helper function It was already what 'git apply' did in read_old_data(), just export it as a real function, and make it be more generic. In particular, this handles the case of the lstat() st_size data not matching the readlink() return value properly (which apparently happens at least on NTFS under Linux). But as a result of this you could also use the new function without even knowing how big the link is going to be, and it will allocate an appropriately sized buffer. So we pass in the st_size of the link as just a hint, rather than a fixed requirement. Signed-off-by: Linus Torvalds Signed-off-by: Junio C Hamano --- builtin-apply.c | 6 ++---- strbuf.c | 27 +++++++++++++++++++++++++++ strbuf.h | 1 + 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/builtin-apply.c b/builtin-apply.c index 4c4d1e177..07244b073 100644 --- a/builtin-apply.c +++ b/builtin-apply.c @@ -1559,10 +1559,8 @@ static int read_old_data(struct stat *st, const char *path, struct strbuf *buf) { switch (st->st_mode & S_IFMT) { case S_IFLNK: - strbuf_grow(buf, st->st_size); - if (readlink(path, buf->buf, st->st_size) != st->st_size) - return -1; - strbuf_setlen(buf, st->st_size); + if (strbuf_readlink(buf, path, st->st_size) < 0) + return error("unable to read symlink %s", path); return 0; case S_IFREG: if (strbuf_read_file(buf, path, st->st_size) != st->st_size) diff --git a/strbuf.c b/strbuf.c index 13be67e4d..bdf49544d 100644 --- a/strbuf.c +++ b/strbuf.c @@ -288,6 +288,33 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint) return sb->len - oldlen; } +#define STRBUF_MAXLINK (2*PATH_MAX) + +int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint) +{ + if (hint < 32) + hint = 32; + + while (hint < STRBUF_MAXLINK) { + int len; + + strbuf_grow(sb, hint); + len = readlink(path, sb->buf, hint); + if (len < 0) { + if (errno != ERANGE) + break; + } else if (len < hint) { + strbuf_setlen(sb, len); + return 0; + } + + /* .. the buffer was too small - try again */ + hint *= 2; + } + strbuf_release(sb); + return -1; +} + int strbuf_getline(struct strbuf *sb, FILE *fp, int term) { int ch; diff --git a/strbuf.h b/strbuf.h index b1670d994..89bd36e15 100644 --- a/strbuf.h +++ b/strbuf.h @@ -124,6 +124,7 @@ extern size_t strbuf_fread(struct strbuf *, size_t, FILE *); /* XXX: if read fails, any partial read is undone */ extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint); extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint); +extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint); extern int strbuf_getline(struct strbuf *, FILE *, int); From a60272b38e548f46c5e1d7127cf18a469fefa237 Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Wed, 17 Dec 2008 09:47:27 -0800 Subject: [PATCH 2/8] Make 'ce_compare_link()' use the new 'strbuf_readlink()' This simplifies the code, and also makes ce_compare_link now able to handle filesystems with odd 'st_size' return values for symlinks. Signed-off-by: Linus Torvalds Signed-off-by: Junio C Hamano --- read-cache.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/read-cache.c b/read-cache.c index c14b56231..b1475ffa0 100644 --- a/read-cache.c +++ b/read-cache.c @@ -99,27 +99,21 @@ static int ce_compare_data(struct cache_entry *ce, struct stat *st) static int ce_compare_link(struct cache_entry *ce, size_t expected_size) { int match = -1; - char *target; void *buffer; unsigned long size; enum object_type type; - int len; + struct strbuf sb = STRBUF_INIT; - target = xmalloc(expected_size); - len = readlink(ce->name, target, expected_size); - if (len != expected_size) { - free(target); + if (strbuf_readlink(&sb, ce->name, expected_size)) return -1; - } + buffer = read_sha1_file(ce->sha1, &type, &size); - if (!buffer) { - free(target); - return -1; + if (buffer) { + if (size == sb.len) + match = memcmp(buffer, sb.buf, size); + free(buffer); } - if (size == expected_size) - match = memcmp(buffer, target, size); - free(buffer); - free(target); + strbuf_release(&sb); return match; } From b760d3aa74585a96f0ccc4ce0374528a38be5177 Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Wed, 17 Dec 2008 09:51:53 -0800 Subject: [PATCH 3/8] Make 'index_path()' use 'strbuf_readlink()' This makes us able to properly index symlinks even on filesystems where st_size doesn't match the true size of the link. Signed-off-by: Linus Torvalds Signed-off-by: Junio C Hamano --- sha1_file.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/sha1_file.c b/sha1_file.c index 0e021c5ec..52d1ead15 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -2523,8 +2523,7 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object, int index_path(unsigned char *sha1, const char *path, struct stat *st, int write_object) { int fd; - char *target; - size_t len; + struct strbuf sb = STRBUF_INIT; switch (st->st_mode & S_IFMT) { case S_IFREG: @@ -2537,20 +2536,17 @@ int index_path(unsigned char *sha1, const char *path, struct stat *st, int write path); break; case S_IFLNK: - len = xsize_t(st->st_size); - target = xmalloc(len + 1); - if (readlink(path, target, len + 1) != st->st_size) { + if (strbuf_readlink(&sb, path, st->st_size)) { char *errstr = strerror(errno); - free(target); return error("readlink(\"%s\"): %s", path, errstr); } if (!write_object) - hash_sha1_file(target, len, blob_type, sha1); - else if (write_sha1_file(target, len, blob_type, sha1)) + hash_sha1_file(sb.buf, sb.len, blob_type, sha1); + else if (write_sha1_file(sb.buf, sb.len, blob_type, sha1)) return error("%s: failed to insert into database", path); - free(target); + strbuf_release(&sb); break; case S_IFDIR: return resolve_gitlink_ref(path, "HEAD", sha1); From cf219d8c68ada1aa2855f4862f15753a32d09641 Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Wed, 17 Dec 2008 10:26:13 -0800 Subject: [PATCH 4/8] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()' This makes all tests pass on a system where 'lstat()' has been hacked to return bogus data in st_size for symlinks. Of course, the test coverage isn't complete, but it's a good baseline. Signed-off-by: Linus Torvalds Signed-off-by: Junio C Hamano --- diff.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/diff.c b/diff.c index afefe087b..4b2029caa 100644 --- a/diff.c +++ b/diff.c @@ -1773,19 +1773,17 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only) s->size = xsize_t(st.st_size); if (!s->size) goto empty; - if (size_only) - return 0; if (S_ISLNK(st.st_mode)) { - int ret; - s->data = xmalloc(s->size); - s->should_free = 1; - ret = readlink(s->path, s->data, s->size); - if (ret < 0) { - free(s->data); + struct strbuf sb = STRBUF_INIT; + + if (strbuf_readlink(&sb, s->path, s->size)) goto err_empty; - } + s->data = strbuf_detach(&sb, &s->size); + s->should_free = 1; return 0; } + if (size_only) + return 0; fd = open(s->path, O_RDONLY); if (fd < 0) goto err_empty; From dfab6aaecfe9df67123efc778f6aea4e9814715a Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Wed, 17 Dec 2008 10:31:36 -0800 Subject: [PATCH 5/8] Make 'prepare_temp_file()' ignore st_size for symlinks The code was already set up to not really need it, so this just massages it a bit to remove the use entirely. Signed-off-by: Linus Torvalds Signed-off-by: Junio C Hamano --- diff.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/diff.c b/diff.c index 4b2029caa..f160c1a35 100644 --- a/diff.c +++ b/diff.c @@ -1881,13 +1881,12 @@ static void prepare_temp_file(const char *name, if (S_ISLNK(st.st_mode)) { int ret; char buf[PATH_MAX + 1]; /* ought to be SYMLINK_MAX */ - size_t sz = xsize_t(st.st_size); - if (sizeof(buf) <= st.st_size) - die("symlink too long: %s", name); - ret = readlink(name, buf, sz); + ret = readlink(name, buf, sizeof(buf)); if (ret < 0) die("readlink(%s)", name); - prep_temp_blob(temp, buf, sz, + if (ret == sizeof(buf)) + die("symlink too long: %s", name); + prep_temp_blob(temp, buf, ret, (one->sha1_valid ? one->sha1 : null_sha1), (one->sha1_valid ? From 737e31af7ae365d6251bb196e06d18afda9c98a8 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Wed, 17 Dec 2008 12:37:48 -0800 Subject: [PATCH 6/8] make_absolute_path(): check bounds when seeing an overlong symlink Signed-off-by: Junio C Hamano Acked-by: Linus Torvalds --- abspath.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/abspath.c b/abspath.c index 8194ce125..649f34f83 100644 --- a/abspath.c +++ b/abspath.c @@ -64,6 +64,8 @@ const char *make_absolute_path(const char *path) len = readlink(buf, next_buf, PATH_MAX); if (len < 0) die ("Invalid symlink: %s", buf); + if (PATH_MAX <= len) + die("symbolic link too long: %s", buf); next_buf[len] = '\0'; buf = next_buf; buf_index = 1 - buf_index; From edfd45d2a0fc85d55479e13e8e3dc7e975a313ce Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Wed, 17 Dec 2008 12:37:53 -0800 Subject: [PATCH 7/8] builtin-blame.c: use strbuf_readlink() When faking a commit out of the work tree contents, use strbuf_readlink() to read the contents of symbolic links. Signed-off-by: Junio C Hamano Acked-by: Linus Torvalds --- builtin-blame.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/builtin-blame.c b/builtin-blame.c index a0d60145f..aae14ef8b 100644 --- a/builtin-blame.c +++ b/builtin-blame.c @@ -1996,7 +1996,6 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con if (!contents_from || strcmp("-", contents_from)) { struct stat st; const char *read_from; - unsigned long fin_size; if (contents_from) { if (stat(contents_from, &st) < 0) @@ -2008,7 +2007,6 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con die("Cannot lstat %s", path); read_from = path; } - fin_size = xsize_t(st.st_size); mode = canon_mode(st.st_mode); switch (st.st_mode & S_IFMT) { case S_IFREG: @@ -2016,9 +2014,8 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con die("cannot open or read %s", read_from); break; case S_IFLNK: - if (readlink(read_from, buf.buf, buf.alloc) != fin_size) + if (strbuf_readlink(&buf, read_from, st.st_size) < 0) die("cannot readlink %s", read_from); - buf.len = fin_size; break; default: die("unsupported file type %s", read_from); From 912342d9d69a30e49491f3fd4d3aa8dc2f6050f3 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Wed, 17 Dec 2008 12:37:58 -0800 Subject: [PATCH 8/8] combine-diff.c: use strbuf_readlink() When showing combined diff using work tree contents, use strbuf_readlink() to read symbolic links. Signed-off-by: Junio C Hamano Acked-by: Linus Torvalds --- combine-diff.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/combine-diff.c b/combine-diff.c index ec8df39bb..bccc018ab 100644 --- a/combine-diff.c +++ b/combine-diff.c @@ -703,15 +703,15 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent, goto deleted_file; if (S_ISLNK(st.st_mode)) { - size_t len = xsize_t(st.st_size); - result_size = len; - result = xmalloc(len + 1); - if (result_size != readlink(elem->path, result, len)) { + struct strbuf buf = STRBUF_INIT; + + if (strbuf_readlink(&buf, elem->path, st.st_size) < 0) { error("readlink(%s): %s", elem->path, strerror(errno)); return; } - result[len] = 0; + result_size = buf.len; + result = strbuf_detach(&buf, NULL); elem->mode = canon_mode(st.st_mode); } else if (0 <= (fd = open(elem->path, O_RDONLY)) &&