[PATCH 0/5] Be careful about lstat()-vs-readlink()

[PATCH 0/5] Be careful about lstat()-vs-readlink()

[Linus Torvalds]

This series of five patches makes us a lot more careful about readlink(), 
and in particular it avoids the code that depends on doing an lstat(), and 
then using st_size as the length of the link. That's what POSIX says 
_should_ happen, but Ramon Tayag reported git failing on NTFS under Linux 
because st_size doesn't match readlink() return value.

It doesn't seem to be unknown elsewhere either, since coreutils (through 
gnulib's areadlink_with_size) also has a big compatibility layer around 
readlink().

This series has been 'tested' by running it with the appended totally 
hacky patch that fakes out the lstat() st_size values for symlinks, so it 
has actually had some testing. 

The complete series does:

Linus Torvalds (5):
  Add generic 'strbuf_readlink()' helper function
  Make 'ce_compare_link()' use the new 'strbuf_readlink()'
  Make 'index_path()' use 'strbuf_readlink()'
  Make 'diff_populate_filespec()' use the new 'strbuf_readlink()'
  Make 'prepare_temp_file()' ignore st_size for symlinks

 builtin-apply.c |    6 ++----
 diff.c          |   25 +++++++++++--------------
 read-cache.c    |   22 ++++++++--------------
 sha1_file.c     |   14 +++++---------
 strbuf.c        |   28 ++++++++++++++++++++++++++++
 strbuf.h        |    1 +
 6 files changed, 55 insertions(+), 41 deletions(-)

and the hacky test-patch (which is obviously _not_ meant to be applied) is 
as follows...

		Linus

---
diff --git a/cache.h b/cache.h
index 231c06d..2d85fca 100644
--- a/cache.h
+++ b/cache.h
@@ -943,4 +943,7 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix);
 char *alias_lookup(const char *alias);
 int split_cmdline(char *cmdline, const char ***argv);
 
+extern int gitlstat(const char *, struct stat *);
+#define lstat gitlstat
+
 #endif /* CACHE_H */
diff --git a/read-cache.c b/read-cache.c
index b1475ff..defbb20 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -15,6 +15,16 @@
 #include "revision.h"
 #include "blob.h"
 
+#undef lstat
+int gitlstat(const char *path, struct stat *buf)
+{
+	int retval = lstat(path, buf);
+	if (!retval && S_ISLNK(buf->st_mode))
+		buf->st_size = 1;
+	return retval;
+}
+#define lstat gitlstat
+
 /* Index extensions.
  *
  * The first letter should be 'A'..'Z' for extensions that are not

[PATCH 1/5] Add generic 'strbuf_readlink()' helper function

[Linus Torvalds]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Wed, 17 Dec 2008 09:36:40 -0800

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 <torvalds@linux-foundation.org>
---
 builtin-apply.c |    6 ++----
 strbuf.c        |   28 ++++++++++++++++++++++++++++
 strbuf.h        |    1 +
 3 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 4c4d1e1..07244b0 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 13be67e..904a2b0 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -288,6 +288,34 @@ 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;
+		continue;
+	}
+	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 b1670d9..89bd36e 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);
 
-- 
1.6.1.rc3.3.gcc3e3

[PATCH 2/5] Make 'ce_compare_link()' use the new 'strbuf_readlink()'

[Linus Torvalds]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Wed, 17 Dec 2008 09:47:27 -0800

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 <torvalds@linux-foundation.org>
---
 read-cache.c |   22 ++++++++--------------
 1 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index c14b562..b1475ff 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;
 }
 
-- 
1.6.1.rc3.3.gcc3e3

[PATCH 3/5] Make 'index_path()' use 'strbuf_readlink()'

[Linus Torvalds]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Wed, 17 Dec 2008 09:51:53 -0800

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 <torvalds@linux-foundation.org>
---
 sha1_file.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 0e021c5..52d1ead 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);
-- 
1.6.1.rc3.3.gcc3e3

[PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()'

[Linus Torvalds]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Wed, 17 Dec 2008 10:26:13 -0800

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 <torvalds@linux-foundation.org>
---
 diff.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/diff.c b/diff.c
index afefe08..4b2029c 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;
-- 
1.6.1.rc3.3.gcc3e3

[PATCH 5/5] Make 'prepare_temp_file()' ignore st_size for symlinks

[Linus Torvalds]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Wed, 17 Dec 2008 10:31:36 -0800

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 <torvalds@linux-foundation.org>
---

There's a few raw 'readlink()' calls left, but they all seem to just have 
a big buffer and rely on the return value of readlink() rather than look 
too closely at st_size. But it would probably be good if somebody 
double-checked it all. 

Even so, this should all be better than what we used to have, though.

 diff.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/diff.c b/diff.c
index 4b2029c..f160c1a 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 ?
-- 
1.6.1.rc3.3.gcc3e3

Re: [PATCH 3/5] Make 'index_path()' use 'strbuf_readlink()'

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> @@ -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);

Thanks; as strbuf_readlink() does not do any iffy library calls that would
stomp on errno, the error reporting should still be valid here.

Re: [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()'

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> diff --git a/diff.c b/diff.c
> index afefe08..4b2029c 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)) {
>  ...
>  		}
> +		if (size_only)
> +			return 0;

It is unfortunate that we need to always readlink even when we only would
want to cull differences early (e.g. --raw without any fancy filters such
as rename detection), but symbolic links should be minorities in any sane
repo, and it should not be worth trying to optimize this for sane
filesystems by making it conditional.

[PATCH 6/5] make_absolute_path(): check bounds when seeing an overlong symlink

[Junio C Hamano]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 abspath.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git i/abspath.c w/abspath.c
index 8194ce1..649f34f 100644
--- i/abspath.c
+++ w/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;

[PATCH 7/5] builtin-blame.c: use strbuf_readlink()

[Junio C Hamano]

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 <gitster@pobox.com>
---
 builtin-blame.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git i/builtin-blame.c w/builtin-blame.c
index a0d6014..aae14ef 100644
--- i/builtin-blame.c
+++ w/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);

[PATCH 8/5] combine-diff.c: use strbuf_readlink()

[Junio C Hamano]

When showing combined diff using work tree contents, use strbuf_readlink()
to read symbolic links.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 combine-diff.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git i/combine-diff.c w/combine-diff.c
index ec8df39..bccc018 100644
--- i/combine-diff.c
+++ w/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)) &&

Re: [PATCH 8/5] combine-diff.c: use strbuf_readlink()

[Linus Torvalds]

On Wed, 17 Dec 2008, Junio C Hamano wrote:
> -			result[len] = 0;
> +			result_size = buf.len;
> +			result = strbuf_detach(&buf, NULL);

If result_size was made size_t, this would be

	result = strbuf_detach(&buf, &result_size);

But whether it makes any difference, I dunno.

Anyway, Ack on the 6-8 additions.

		Linus

Re: [PATCH 1/5] Add generic 'strbuf_readlink()' helper function

[Jay Soffian]

On Wed, Dec 17, 2008 at 1:42 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> +       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;
> +               continue;
> +       }

Why the continue statement at the end of the loop?

j.

Re: [PATCH 8/5] combine-diff.c: use strbuf_readlink()

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Wed, 17 Dec 2008, Junio C Hamano wrote:
>> -			result[len] = 0;
>> +			result_size = buf.len;
>> +			result = strbuf_detach(&buf, NULL);
>
> If result_size was made size_t, this would be
>
> 	result = strbuf_detach(&buf, &result_size);
>
> But whether it makes any difference, I dunno.

Yeah, use of "unsigned long" where "size_t" could be more appropriate
stems from the very initial commit e83c516 (Initial revision of "git", the
information manager from hell, 2005-04-07) and it is everywhere, and
updating them one by one like you suggest would take forever ;-)

Perhaps libgit2 would settle with a better typing system.  The original
draft by Shawn looked a bit too overengineered in its use of typedefs, but
if I recall correctly later revisions were made saner.  I haven't checked
its current status.

Re: [PATCH 1/5] Add generic 'strbuf_readlink()' helper function

[Linus Torvalds]

On Wed, 17 Dec 2008, Jay Soffian wrote:
> > +               /* .. the buffer was too small - try again */
> > +               hint *= 2;
> > +               continue;
> > +       }
> 
> Why the continue statement at the end of the loop?

Oh, it's unnecessary left-over from me originally writing that part inside 
an if-statement, and then moving things around.

So it could/should be deleted. Good eyes.

		Linus

Re: [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()'

[Mark Burton]

Howdy folks,

When I compile this latest version of diff.c on a i686 dual-core Pentium box
I see:

diff.c: In function ‘diff_populate_filespec’:
diff.c:1781: warning: passing argument 2 of ‘strbuf_detach’ from incompatible pointer type

The same code compiles without warning on a x86_64 AMD box. Both
machines are running stock Ubuntu 8.04.

Does it need a cast on some architectures?

Cheers,

Mark

Re: [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()'

[Linus Torvalds]

On Thu, 18 Dec 2008, Mark Burton wrote:
> 
> Does it need a cast on some architectures?

Gaah. My bad. It should work fine ("unsigned long" is physically the same 
type as "size_t" in your case), but on 32-bit x86, size_t is generally 
"unsigned int" - which is the same physical type there (both int and long 
are 32-bit) but causes a valid warning.

I think we should just make the "size" member "size_t". I use "unsigned 
long" out of much too long habit, since we traditionally avoided "size_t" 
in the kernel due to it just being another unnecessary architecture- 
specific detail.

So the proper patch is probably just the following. Sorry about that,

		Linus
---
 diffcore.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/diffcore.h b/diffcore.h
index 5b63458..16a73e6 100644
--- a/diffcore.h
+++ b/diffcore.h
@@ -30,7 +30,7 @@ struct diff_filespec {
 	void *data;
 	void *cnt_data;
 	const char *funcname_pattern_ident;
-	unsigned long size;
+	size_t size;
 	int count;               /* Reference count */
 	int xfrm_flags;		 /* for use by the xfrm */
 	int rename_used;         /* Count of rename users */

Re: [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()'

[René Scharfe]

Mark Burton schrieb:
> Howdy folks,
> 
> When I compile this latest version of diff.c on a i686 dual-core Pentium box
> I see:
> 
> diff.c: In function ‘diff_populate_filespec’:
> diff.c:1781: warning: passing argument 2 of ‘strbuf_detach’ from incompatible pointer type
> 
> The same code compiles without warning on a x86_64 AMD box. Both
> machines are running stock Ubuntu 8.04.
> 
> Does it need a cast on some architectures?

The type of the size member of struct stat is off_t, while strbuf_detach expects
a size_t pointer.  This patch should fix the warning:

diff --git a/diff.c b/diff.c
index f160c1a..0484601 100644
--- a/diff.c
+++ b/diff.c
@@ -1778,7 +1778,8 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
 
 			if (strbuf_readlink(&sb, s->path, s->size))
 				goto err_empty;
-			s->data = strbuf_detach(&sb, &s->size);
+			s->size = sb.len;
+			s->data = strbuf_detach(&sb, NULL);
 			s->should_free = 1;
 			return 0;
 		}

Re: [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()'

[René Scharfe]

René Scharfe schrieb:
> Mark Burton schrieb:
>> Howdy folks,
>>
>> When I compile this latest version of diff.c on a i686 dual-core Pentium box
>> I see:
>>
>> diff.c: In function ‘diff_populate_filespec’:
>> diff.c:1781: warning: passing argument 2 of ‘strbuf_detach’ from incompatible pointer type
>>
>> The same code compiles without warning on a x86_64 AMD box. Both
>> machines are running stock Ubuntu 8.04.
>>
>> Does it need a cast on some architectures?
> 
> The type of the size member of struct stat is off_t, while strbuf_detach expects
> a size_t pointer.  This patch should fix the warning:

Nevermind, I somehow missed the last "t" in line 1760..  The patch works
nevertheless. 8-)

René

Re: [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()'

[René Scharfe]

Linus Torvalds schrieb:
> 
> On Thu, 18 Dec 2008, Mark Burton wrote:
>> Does it need a cast on some architectures?
> 
> Gaah. My bad. It should work fine ("unsigned long" is physically the same 
> type as "size_t" in your case), but on 32-bit x86, size_t is generally 
> "unsigned int" - which is the same physical type there (both int and long 
> are 32-bit) but causes a valid warning.
> 
> I think we should just make the "size" member "size_t". I use "unsigned 
> long" out of much too long habit, since we traditionally avoided "size_t" 
> in the kernel due to it just being another unnecessary architecture- 
> specific detail.
> 
> So the proper patch is probably just the following. Sorry about that,
> 
> 		Linus
> ---
>  diffcore.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/diffcore.h b/diffcore.h
> index 5b63458..16a73e6 100644
> --- a/diffcore.h
> +++ b/diffcore.h
> @@ -30,7 +30,7 @@ struct diff_filespec {
>  	void *data;
>  	void *cnt_data;
>  	const char *funcname_pattern_ident;
> -	unsigned long size;
> +	size_t size;
>  	int count;               /* Reference count */
>  	int xfrm_flags;		 /* for use by the xfrm */
>  	int rename_used;         /* Count of rename users */

Yes, but now I get two new warnings:

diff.c: In function `diff_populate_filespec':
diff.c:1809: warning: passing arg 2 of `sha1_object_info' from
incompatible pointer type
diff.c:1811: warning: passing arg 3 of `read_sha1_file' from
incompatible pointer type

If we followed that way along we'd convert just about everything to use
size_t, which is going a bit too far during the -rc phase..

René


PS: In the other subthread, I was missing the "t" in "st" in line 1757,
not 1760.  Ahem.

Re: [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()'

[Linus Torvalds]

On Thu, 18 Dec 2008, René Scharfe wrote:
> 
> Yes, but now I get two new warnings:
> 
> diff.c: In function `diff_populate_filespec':
> diff.c:1809: warning: passing arg 2 of `sha1_object_info' from
> incompatible pointer type
> diff.c:1811: warning: passing arg 3 of `read_sha1_file' from
> incompatible pointer type

Yeah, yeah, we should probably fix it all, but you're right, your patch 
avoids the pain.

The good news is that "size_t" and "unsigned long" really are the same 
size on all sane platforms. You'll see differences just in the odd 16-bit 
world ("small" memory model with 16-bit size_t and possibly 32-bit "long") 

Although I guess some really odd more modern case might have a 32-bit 
pointer (and size_t) and 64-bit long. If it's possible to screw the type 
system up, history shows that somebody (usually Windows) will do it.

		Linus

Re: [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()'

[Olivier Galibert]

On Thu, Dec 18, 2008 at 09:49:23AM -0800, Linus Torvalds wrote:
> If it's possible to screw the type system up, history shows that
> somebody (usually Windows) will do it.

AFAIK, the Win64 long is 32 bits and size_t/void * is 64 bits.  I'll
leave you to your groaning.

  OG.

[PATCH] diff.c: fix pointer type warning

[René Scharfe]

As Mark Burton noted, the conversion to strbuf_readlink() caused a
compile warning on some architectures:

> diff.c: In function ‘diff_populate_filespec’:
> diff.c:1781: warning: passing argument 2 of ‘strbuf_detach’ from incompatible pointer type

A pointer to an unsigned long is given while a pointer to a size_t is
expected; the two types are not considered to be equivalent everywhere.

The real fix would be to change the type of the size member of struct
diff_filespec to size_t, but that would cause other warnings in
connection with functions expecting unsigned long, and attempts to fix
them might loose an avalanche of changes.  Later.  This patch just
silences the warning by adding an (implicit) casting step.

Reported-by: Mark Burton <markb@ordern.com>
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 diff.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/diff.c b/diff.c
index f160c1a..0484601 100644
--- a/diff.c
+++ b/diff.c
@@ -1778,7 +1778,8 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
 
 			if (strbuf_readlink(&sb, s->path, s->size))
 				goto err_empty;
-			s->data = strbuf_detach(&sb, &s->size);
+			s->size = sb.len;
+			s->data = strbuf_detach(&sb, NULL);
 			s->should_free = 1;
 			return 0;
 		}

Re: [PATCH] diff.c: fix pointer type warning

[Junio C Hamano]

Thanks; I think I already have it in my tree from your yesterday's
e-mail.  I just have been too busy to whip the other branches into shape
to push the results out.

[PATCH] strbuf_readlink semantics update.

[Pierre Habouzit]

strbuf_* operations are meant to append their results to a current buffer
rather than _replace_ its content. Modify strbuf_readlink accordingly.

Current callers only operate on empty buffers at the moment and this
semantic change doesn't break any current code.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 strbuf.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index bdf4954..254a7ee 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -299,12 +299,12 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
 		int len;
 
 		strbuf_grow(sb, hint);
-		len = readlink(path, sb->buf, hint);
+		len = readlink(path, sb->buf + sb->len, hint);
 		if (len < 0) {
 			if (errno != ERANGE)
 				break;
 		} else if (len < hint) {
-			strbuf_setlen(sb, len);
+			strbuf_setlen(sb, sb->len + len);
 			return 0;
 		}
 
-- 
1.6.1.rc4.304.g3da087

Re: [PATCH] strbuf_readlink semantics update.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 4021 bytes --]

when readlink fails, the strbuf shall not be destroyed. It's not how
read_file_or_gitlink works for example.

Fix strbuf_readlink callers to destroy the buffer when appropriate.

Fix read_old_data possible leaks in case of errors, since even when no
data has been read, the strbufs may have grown to prepare the reads.
strbuf_release must be called on them.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---

  I know it somehow add lines to the callers, but it's actually more
  important to keep consistency among the strbuf APIs than to save 10
  SLOCs.


 builtin-apply.c |    8 ++++++--
 combine-diff.c  |    1 +
 diff.c          |    4 +++-
 read-cache.c    |    4 +++-
 sha1_file.c     |    1 +
 strbuf.c        |    2 +-
 6 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 07244b0..c1fe9ca 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2306,8 +2306,10 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *
 		/* We have a patched copy in memory use that */
 		strbuf_add(&buf, tpatch->result, tpatch->resultsize);
 	} else if (cached) {
-		if (read_file_or_gitlink(ce, &buf))
+		if (read_file_or_gitlink(ce, &buf)) {
+			strbuf_release(&buf);
 			return error("read of %s failed", patch->old_name);
+		}
 	} else if (patch->old_name) {
 		if (S_ISGITLINK(patch->old_mode)) {
 			if (ce) {
@@ -2320,8 +2322,10 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *
 				patch->fragments = NULL;
 			}
 		} else {
-			if (read_old_data(st, patch->old_name, &buf))
+			if (read_old_data(st, patch->old_name, &buf)) {
+				strbuf_release(&buf);
 				return error("read of %s failed", patch->old_name);
+			}
 		}
 	}
 
diff --git a/combine-diff.c b/combine-diff.c
index bccc018..674745d 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -706,6 +706,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			struct strbuf buf = STRBUF_INIT;
 
 			if (strbuf_readlink(&buf, elem->path, st.st_size) < 0) {
+				strbuf_release(&buf);
 				error("readlink(%s): %s", elem->path,
 				      strerror(errno));
 				return;
diff --git a/diff.c b/diff.c
index b57d9ac..41f7e1c 100644
--- a/diff.c
+++ b/diff.c
@@ -1778,8 +1778,10 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
 		if (S_ISLNK(st.st_mode)) {
 			struct strbuf sb = STRBUF_INIT;
 
-			if (strbuf_readlink(&sb, s->path, s->size))
+			if (strbuf_readlink(&sb, s->path, s->size)) {
+				strbuf_release(&sb);
 				goto err_empty;
+			}
 			s->size = sb.len;
 			s->data = strbuf_detach(&sb, NULL);
 			s->should_free = 1;
diff --git a/read-cache.c b/read-cache.c
index db166da..9673d91 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -104,8 +104,10 @@ static int ce_compare_link(struct cache_entry *ce, size_t expected_size)
 	enum object_type type;
 	struct strbuf sb = STRBUF_INIT;
 
-	if (strbuf_readlink(&sb, ce->name, expected_size))
+	if (strbuf_readlink(&sb, ce->name, expected_size)) {
+		strbuf_release(&sb);
 		return -1;
+	}
 
 	buffer = read_sha1_file(ce->sha1, &type, &size);
 	if (buffer) {
diff --git a/sha1_file.c b/sha1_file.c
index 52d1ead..a62b53d 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2538,6 +2538,7 @@ int index_path(unsigned char *sha1, const char *path, struct stat *st, int write
 	case S_IFLNK:
 		if (strbuf_readlink(&sb, path, st->st_size)) {
 			char *errstr = strerror(errno);
+			strbuf_release(&sb);
 			return error("readlink(\"%s\"): %s", path,
 			             errstr);
 		}
diff --git a/strbuf.c b/strbuf.c
index 254a7ee..b1f2a97 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -311,7 +311,7 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
 		/* .. the buffer was too small - try again */
 		hint *= 2;
 	}
-	strbuf_release(sb);
+	sb->buf[sb->len] = '\0';
 	return -1;
 }
 
-- 
1.6.1.rc4.306.g849c2


[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] strbuf_readlink semantics update.

[Linus Torvalds]

On Tue, 23 Dec 2008, Pierre Habouzit wrote:
>
> when readlink fails, the strbuf shall not be destroyed. It's not how
> read_file_or_gitlink works for example.

I disagree.

This patch just makes things worse. Just leave the "strbuf_release()" in 
_one_ place.

Look:

   6 files changed, 15 insertions(+), 5 deletions(-)

you added ten unnecessary lines, and you made the interface harder to use. 
What was the gain here?

> Fix read_old_data possible leaks in case of errors, since even when no
> data has been read, the strbufs may have grown to prepare the reads.
> strbuf_release must be called on them.

That's a separate error, and quite frankly, the best approach to that is 
likely to instead of breaking strbuf_readlink(), just make the S_IFREG() 
case release it.

I'd suggest that strbuf_read_file() should probably also do a 
strbuf_release() if it returns a negative error value, but that's a 
separate issue (and still leaves "read_old_data()" having to release 
things, since read_old_data() wants to see exactly st_size bytes. Although 
I suspect we might want to change that, and just make it test for 
negative too).

		Linus

Re: [PATCH] strbuf_readlink semantics update.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1420 bytes --]

On Tue, Dec 23, 2008 at 06:16:01PM +0000, Linus Torvalds wrote:
> 
> 
> On Tue, 23 Dec 2008, Pierre Habouzit wrote:
> >
> > when readlink fails, the strbuf shall not be destroyed. It's not how
> > read_file_or_gitlink works for example.
> 
> I disagree.
> 
> This patch just makes things worse. Just leave the "strbuf_release()" in 
> _one_ place.

The "problem" is that the strbuf API usually works that way: functions
append things to a buffer, or do nothing, but always keep the buffer in
a state where you can append more stuff to it.

If read_file_or_gitlink or strbuf_readlink destroy the buffer, then you
break the second expectation people (should) have about the strbuf API.

The reason is that if you built things in the buffer, you really don't
want it to be undone just because the last bit you add went wrong for
some reason. Or if you have a buffer that is reused in a loop, you don't
want the buffer you allocated to be dropped just because one error
occurred.


Alternatively, we could pass a flag to tell function performing reads
(fread, read, readlink, whatever) that those should destroy the buffer
on error or just report it. I don't really know. It sounds like
over-engineering though.


-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] strbuf_readlink semantics update.

[René Scharfe]

Pierre Habouzit schrieb:
> On Tue, Dec 23, 2008 at 06:16:01PM +0000, Linus Torvalds wrote:
>>
>> On Tue, 23 Dec 2008, Pierre Habouzit wrote:
>>> when readlink fails, the strbuf shall not be destroyed. It's not how
>>> read_file_or_gitlink works for example.
>> I disagree.
>>
>> This patch just makes things worse. Just leave the "strbuf_release()" in 
>> _one_ place.
> 
> The "problem" is that the strbuf API usually works that way: functions
> append things to a buffer, or do nothing, but always keep the buffer in
> a state where you can append more stuff to it.
> 
> If read_file_or_gitlink or strbuf_readlink destroy the buffer, then you
> break the second expectation people (should) have about the strbuf API.

The "append or do nothing" rule is broken by strbuf_getline(), but I agree
to your reasoning.  How about refining this rule a bit to "do your thing
and roll back changes if an error occurs"?  I think it's not worth to undo
allocation extensions, but making reverting first time allocations seems
like a good idea.  Something like this?

René


PS: only nine lines! ;-)


 strbuf.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index bdf4954..6ed0684 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -256,18 +256,21 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder,
 size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)
 {
 	size_t res;
+	size_t oldalloc = sb->alloc;
 
 	strbuf_grow(sb, size);
 	res = fread(sb->buf + sb->len, 1, size, f);
-	if (res > 0) {
+	if (res > 0)
 		strbuf_setlen(sb, sb->len + res);
-	}
+	else if (res < 0 && oldalloc == 0)
+		strbuf_release(sb);
 	return res;
 }
 
 ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
 {
 	size_t oldlen = sb->len;
+	size_t oldalloc = sb->alloc;
 
 	strbuf_grow(sb, hint ? hint : 8192);
 	for (;;) {
@@ -275,7 +278,10 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
 
 		cnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);
 		if (cnt < 0) {
-			strbuf_setlen(sb, oldlen);
+			if (oldalloc == 0)
+				strbuf_release(sb);
+			else
+				strbuf_setlen(sb, oldlen);
 			return -1;
 		}
 		if (!cnt)
@@ -292,6 +298,8 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
 
 int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
 {
+	size_t oldalloc = sb->alloc;
+
 	if (hint < 32)
 		hint = 32;
 
@@ -311,7 +319,8 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
 		/* .. the buffer was too small - try again */
 		hint *= 2;
 	}
-	strbuf_release(sb);
+	if (oldalloc == 0)
+		strbuf_release(sb);
 	return -1;
 }
 

Re: [PATCH] strbuf_readlink semantics update.

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Pierre Habouzit schrieb:
>> On Tue, Dec 23, 2008 at 06:16:01PM +0000, Linus Torvalds wrote:
>>>
>>> On Tue, 23 Dec 2008, Pierre Habouzit wrote:
>>>> when readlink fails, the strbuf shall not be destroyed. It's not how
>>>> read_file_or_gitlink works for example.
>>> I disagree.
>>>
>>> This patch just makes things worse. Just leave the "strbuf_release()" in 
>>> _one_ place.
> ...
> The "append or do nothing" rule is broken by strbuf_getline(), but I agree
> to your reasoning.  How about refining this rule a bit to "do your thing
> and roll back changes if an error occurs"?  I think it's not worth to undo
> allocation extensions, but making reverting first time allocations seems
> like a good idea.  Something like this?

I think this is much better than Pierre's.  Pierre's "if it is called
strbuf_*, it should always append" is a good uniformity to have in an API,
but making the caller suffer for clean-up is going backwards.  The reason
we use strbuf when we can is so that the callers do not have to worry
about memory allocation issues too much.

Re: [PATCH] strbuf_readlink semantics update.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1520 bytes --]

On Thu, Dec 25, 2008 at 07:23:58AM +0000, Junio C Hamano wrote:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> 
> > Pierre Habouzit schrieb:
> >> On Tue, Dec 23, 2008 at 06:16:01PM +0000, Linus Torvalds wrote:
> >>>
> >>> On Tue, 23 Dec 2008, Pierre Habouzit wrote:
> >>>> when readlink fails, the strbuf shall not be destroyed. It's not how
> >>>> read_file_or_gitlink works for example.
> >>> I disagree.
> >>>
> >>> This patch just makes things worse. Just leave the "strbuf_release()" in 
> >>> _one_ place.
> > ...
> > The "append or do nothing" rule is broken by strbuf_getline(), but I agree
> > to your reasoning.  How about refining this rule a bit to "do your thing
> > and roll back changes if an error occurs"?  I think it's not worth to undo
> > allocation extensions, but making reverting first time allocations seems
> > like a good idea.  Something like this?
> 
> I think this is much better than Pierre's.

I agree it's a fine semantics.

> Pierre's "if it is called strbuf_*, it should always append" is a good
> uniformity to have in an API, but making the caller suffer for
> clean-up is going backwards.  The reason we use strbuf when we can is
> so that the callers do not have to worry about memory allocation
> issues too much.

Ack.

Sorry for the delay I was on vacation.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

[PATCH] strbuf: instate cleanup rule in case of non-memory errors

[René Scharfe]

Make all strbuf functions that can fail free() their memory on error if
they have allocated it.  They don't shrink buffers that have been grown,
though.

This allows for easier error handling, as callers only need to call
strbuf_release() if A) the command succeeded or B) if they would have had
to do so anyway because they added something to the strbuf themselves.

Bonus hunk: document strbuf_readlink.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 Documentation/technical/api-strbuf.txt |   11 +++++++++--
 strbuf.c                               |   17 +++++++++++++----
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
index a8ee2fe..9a4e3ea 100644
--- a/Documentation/technical/api-strbuf.txt
+++ b/Documentation/technical/api-strbuf.txt
@@ -133,8 +133,10 @@ Functions
 
 * Adding data to the buffer
 
-NOTE: All of these functions in this section will grow the buffer as
-      necessary.
+NOTE: All of the functions in this section will grow the buffer as necessary.
+If they fail for some reason other than memory shortage and the buffer hadn't
+been allocated before (i.e. the `struct strbuf` was set to `STRBUF_INIT`),
+then they will free() it.
 
 `strbuf_addch`::
 
@@ -235,6 +237,11 @@ same behaviour as well.
 	Read the contents of a file, specified by its path. The third argument
 	can be used to give a hint about the file size, to avoid reallocs.
 
+`strbuf_readlink`::
+
+	Read the target of a symbolic link, specified by its path.  The third
+	argument can be used to give a hint about the size, to avoid reallocs.
+
 `strbuf_getline`::
 
 	Read a line from a FILE* pointer. The second argument specifies the line
diff --git a/strbuf.c b/strbuf.c
index bdf4954..6ed0684 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -256,18 +256,21 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder,
 size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)
 {
 	size_t res;
+	size_t oldalloc = sb->alloc;
 
 	strbuf_grow(sb, size);
 	res = fread(sb->buf + sb->len, 1, size, f);
-	if (res > 0) {
+	if (res > 0)
 		strbuf_setlen(sb, sb->len + res);
-	}
+	else if (res < 0 && oldalloc == 0)
+		strbuf_release(sb);
 	return res;
 }
 
 ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
 {
 	size_t oldlen = sb->len;
+	size_t oldalloc = sb->alloc;
 
 	strbuf_grow(sb, hint ? hint : 8192);
 	for (;;) {
@@ -275,7 +278,10 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
 
 		cnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);
 		if (cnt < 0) {
-			strbuf_setlen(sb, oldlen);
+			if (oldalloc == 0)
+				strbuf_release(sb);
+			else
+				strbuf_setlen(sb, oldlen);
 			return -1;
 		}
 		if (!cnt)
@@ -292,6 +298,8 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
 
 int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
 {
+	size_t oldalloc = sb->alloc;
+
 	if (hint < 32)
 		hint = 32;
 
@@ -311,7 +319,8 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
 		/* .. the buffer was too small - try again */
 		hint *= 2;
 	}
-	strbuf_release(sb);
+	if (oldalloc == 0)
+		strbuf_release(sb);
 	return -1;
 }
 
-- 
1.6.1

Re: [PATCH] strbuf: instate cleanup rule in case of non-memory errors

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Make all strbuf functions that can fail free() their memory on error if
> they have allocated it.  They don't shrink buffers that have been grown,
> though.

Thanks; applied.