[AGGREGATED PATCH] Fix in-place editing functions in convert.c

[AGGREGATED PATCH] Fix in-place editing functions in convert.c

[Pierre Habouzit]

* crlf_to_git and ident_to_git:

  Don't grow the buffer if there is enough space in the first place.
  As a side effect, when the editing is done "in place", we don't grow, so
  the buffer pointer doesn't changes, and `src' isn't invalidated anymore.

  Thanks to Bernt Hansen for the bug report.

* apply_filter:

  Fix memory leak due to fake in-place editing that didn't collected the
  old buffer when the filter succeeds. Also a cosmetic fix.

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

This patch is on top of master, and supersedes both patch I sent before.
Following dscho's remark, I only grow the buffer if they aren't big enough
in the first place, which ensures that buffers are not touched if edited in
place.

 convert.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/convert.c b/convert.c
index 0d5e909..aa95834 100644
--- a/convert.c
+++ b/convert.c
@@ -110,7 +110,9 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
 			return 0;
 	}
 
-	strbuf_grow(buf, len);
+	/* only grow if not in place */
+	if (strbuf_avail(buf) + buf->len < len)
+		strbuf_grow(buf, len - buf->len);
 	dst = buf->buf;
 	if (action == CRLF_GUESS) {
 		/*
@@ -281,20 +283,19 @@ static int apply_filter(const char *path, const char *src, size_t len,
 		ret = 0;
 	}
 	if (close(pipe_feed[0])) {
-		ret = error("read from external filter %s failed", cmd);
+		error("read from external filter %s failed", cmd);
 		ret = 0;
 	}
 	status = finish_command(&child_process);
 	if (status) {
-		ret = error("external filter %s failed %d", cmd, -status);
+		error("external filter %s failed %d", cmd, -status);
 		ret = 0;
 	}
 
 	if (ret) {
-		*dst = nbuf;
-	} else {
-		strbuf_release(&nbuf);
+		strbuf_swap(dst, &nbuf);
 	}
+	strbuf_release(&nbuf);
 	return ret;
 }
 
@@ -422,7 +423,9 @@ static int ident_to_git(const char *path, const char *src, size_t len,
 	if (!ident || !count_ident(src, len))
 		return 0;
 
-	strbuf_grow(buf, len);
+	/* only grow if not in place */
+	if (strbuf_avail(buf) + buf->len < len)
+		strbuf_grow(buf, len - buf->len);
 	dst = buf->buf;
 	for (;;) {
 		dollar = memchr(src, '$', len);
-- 
1.5.3.4.207.gb504-dirty

[PATCH] Fix in-place editing in crlf_to_git and ident_to_git.

[Pierre Habouzit]

When crlf_to_git or ident_to_git are called "in place", the buffer already
is big enough and should not be resized (as it could make the buffer address
change, hence invalidate the `src' pointers !).

Also fix the growth length at the same time: we want to replace the buffer
content (not append) in those functions as they are filters.

Thanks to Bernt Hansen for the bug report.

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

  This patch is done on top of master, as strbuf's have been merged.
This is a major issue.

 convert.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/convert.c b/convert.c
index 0d5e909..4664197 100644
--- a/convert.c
+++ b/convert.c
@@ -110,7 +110,9 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
 			return 0;
 	}
 
-	strbuf_grow(buf, len);
+	/* only grow if not in place */
+	if (src != buf->buf)
+		strbuf_grow(buf, len - buf->len);
 	dst = buf->buf;
 	if (action == CRLF_GUESS) {
 		/*
@@ -281,12 +283,12 @@ static int apply_filter(const char *path, const char *src, size_t len,
 		ret = 0;
 	}
 	if (close(pipe_feed[0])) {
-		ret = error("read from external filter %s failed", cmd);
+		error("read from external filter %s failed", cmd);
 		ret = 0;
 	}
 	status = finish_command(&child_process);
 	if (status) {
-		ret = error("external filter %s failed %d", cmd, -status);
+		error("external filter %s failed %d", cmd, -status);
 		ret = 0;
 	}
 
@@ -422,7 +424,9 @@ static int ident_to_git(const char *path, const char *src, size_t len,
 	if (!ident || !count_ident(src, len))
 		return 0;
 
-	strbuf_grow(buf, len);
+	/* only grow if not in place */
+	if (src != buf->buf)
+		strbuf_grow(buf, len - buf->len);
 	dst = buf->buf;
 	for (;;) {
 		dollar = memchr(src, '$', len);
-- 
1.5.3.4.207.gc79d4-dirty

[PATCH] Fix memory leak in apply_filter.

[Pierre Habouzit]

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

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

While we're at it... Here is a stupid memory leak in apply_filter.
On top of the previous commit.

 convert.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/convert.c b/convert.c
index 4664197..d5c197f 100644
--- a/convert.c
+++ b/convert.c
@@ -293,10 +293,9 @@ static int apply_filter(const char *path, const char *src, size_t len,
 	}
 
 	if (ret) {
-		*dst = nbuf;
-	} else {
-		strbuf_release(&nbuf);
+		strbuf_swap(dst, &nbuf);
 	}
+	strbuf_release(&nbuf);
 	return ret;
 }
 
-- 
1.5.3.4.208.gdcc67-dirty

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

[PATCH] Fix memory leak in apply_filter.

[Pierre Habouzit]

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

While we're at it... Here is a stupid memory leak in apply_filter.
On top of the previous commit.

The same, repost, unsigned doh.

 convert.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/convert.c b/convert.c
index 4664197..d5c197f 100644
--- a/convert.c
+++ b/convert.c
@@ -293,10 +293,9 @@ static int apply_filter(const char *path, const char *src, size_t len,
 	}
 
 	if (ret) {
-		*dst = nbuf;
-	} else {
-		strbuf_release(&nbuf);
+		strbuf_swap(dst, &nbuf);
 	}
+	strbuf_release(&nbuf);
 	return ret;
 }
 
-- 
1.5.3.4.208.gdcc67-dirty

Re: [PATCH] Fix in-place editing in crlf_to_git and ident_to_git.

[Johannes Schindelin]

Hi,

On Fri, 5 Oct 2007, Pierre Habouzit wrote:

> When crlf_to_git or ident_to_git are called "in place", the buffer 
> already is big enough and should not be resized (as it could make the 
> buffer address change, hence invalidate the `src' pointers !).

I wonder why we resize at all if the buffer is big enough to begin with.

Ciao,
Dscho

Re: [PATCH] Fix in-place editing in crlf_to_git and ident_to_git.

[Pierre Habouzit]

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

On Fri, Oct 05, 2007 at 08:30:45AM +0000, Johannes Schindelin wrote:
> Hi,
> 
> On Fri, 5 Oct 2007, Pierre Habouzit wrote:
> 
> > When crlf_to_git or ident_to_git are called "in place", the buffer 
> > already is big enough and should not be resized (as it could make the 
> > buffer address change, hence invalidate the `src' pointers !).
> 
> I wonder why we resize at all if the buffer is big enough to begin with.

  strbuf_grow takes care of that itself but indeed you make me see that
my patch is wrong. if buf->len > len then len - buf->len is err a bit
big.

  I'll roll better ones.

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

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

Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c

[Johannes Sixt]

Pierre Habouzit schrieb:
> @@ -281,20 +283,19 @@ static int apply_filter(const char *path, const char *src, size_t len,
>  		ret = 0;
>  	}
>  	if (close(pipe_feed[0])) {
> -		ret = error("read from external filter %s failed", cmd);
> +		error("read from external filter %s failed", cmd);
>  		ret = 0;
>  	}
>  	status = finish_command(&child_process);
>  	if (status) {
> -		ret = error("external filter %s failed %d", cmd, -status);
> +		error("external filter %s failed %d", cmd, -status);
>  		ret = 0;
>  	}

If you want to, you can leave away these cosmetical corrections since I'm 
working on a patch that replaces this entire passus. (Will be needed for the 
MinGW port).

-- Hannes

Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c

[Bernt Hansen]

This fixes it for me.  Thanks!!

Bernt

Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c

[Linus Torvalds]

On Fri, 5 Oct 2007, Pierre Habouzit wrote:
>  
> -	strbuf_grow(buf, len);
> +	/* only grow if not in place */
> +	if (strbuf_avail(buf) + buf->len < len)
> +		strbuf_grow(buf, len - buf->len);

Umm. This is really ugly.

The whole point of strbuf's was that you shouldn't be doing your own 
allocation decisions etc. So why do it?

Wouldn't it be much better to have a strbuf_make_room() interface that 
just guarantees that there is enough room fo "len"? 

Otherwise, code like the above would seem to make the whole point of a 
safer string interface rather pointless. The above code only makes sense 
if you know how the strbuf's are internally done, so it should not exists 
except as internal strbuf code. No?

		Linus

Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c

[Pierre Habouzit]

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

On Fri, Oct 05, 2007 at 03:26:44PM +0000, Linus Torvalds wrote:
> 
> 
> On Fri, 5 Oct 2007, Pierre Habouzit wrote:
> >  
> > -	strbuf_grow(buf, len);
> > +	/* only grow if not in place */
> > +	if (strbuf_avail(buf) + buf->len < len)
> > +		strbuf_grow(buf, len - buf->len);
> 
> Umm. This is really ugly.

  I agree.

> The whole point of strbuf's was that you shouldn't be doing your own 
> allocation decisions etc. So why do it?

  the point here is that it's in a "filter" that is called like this:

some_filter(buf->buf, buf->len, buf);
            src       len       dst

  You can call the filter with src/len being data from anywere,
including the current content of the destination buffer.

  Then there is two cases, either the filter is known to be done in
place, either we can't know or we know it wont.

  In the latter case, we have a bit of code like that:

      char *to_free = NULL;

      if (buf->buf == src)
	  to_free = strbuf_detach(&buf);

      .. hack ..
      free(to_free);


  In the former case, then there is a small glitch, being that if we are
doing in place editing, we should not touch buffer at all (or it would
invalidate "src"). If we are not in the in-place editing code though,
then we have to make the resulting buffer be big enough...

> Wouldn't it be much better to have a strbuf_make_room() interface that 
> just guarantees that there is enough room fo "len"? 

  Right, that would do the same btw ;)

> Otherwise, code like the above would seem to make the whole point of a 
> safer string interface rather pointless. The above code only makes sense 
> if you know how the strbuf's are internally done, so it should not exists 
> except as internal strbuf code. No?

  Well, the above code is used in filters to spare reallocations. So if
we want to "blackbox" such a think, strbuf_make_room isn't the proper
API. We should rather use
  void *strbuf_begin_filter(struct strbuf *sb, const char *src, size_t reslen);
  strbuf_end_filter(void *);

  `strbuf_begin_filter` would decide upon the hint `reslen` argument if
we know if we can work in place or not (has a meaning iff src points
into the strbuf buffer). If not, it could stash the strbuf buffer in the
returned void * to be freed at the end of the filter. It seems like a
better alternative than a strbuf_make_room.

  Of course, strbuf_begin_filter() would really be simple and basically
be:

    char *tmp;
    if (src points into sb->buf && reslen > sb->alloc - 1) {
        // in place editing is OK
        return NULL;
    }
    tmp = strbuf_release(&sb);
    strbuf_grow(&sb, len);
    return tmp;


  and strbuf_end_filter would just be "free" :)


  We could even make "reslen" be a ssize_t so that -1 would mean "I've
absolutely no idea how much space I'll need (or just in place editing is
not supported). This way, both hacks I described in this mail could be
hidden in the strbuf module, and be properly documented _and_ safe _and_
efficient.

What do you think ?

[Though if we do that, I still think it's more important to fix the
bug in master, and have a new patch implementing this approach]
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

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

Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c

[Sam Ravnborg]

On Fri, Oct 05, 2007 at 08:26:44AM -0700, Linus Torvalds wrote:
> 
> 
> On Fri, 5 Oct 2007, Pierre Habouzit wrote:
> >  
> > -	strbuf_grow(buf, len);
> > +	/* only grow if not in place */
> > +	if (strbuf_avail(buf) + buf->len < len)
> > +		strbuf_grow(buf, len - buf->len);
> 
> Umm. This is really ugly.
> 
> The whole point of strbuf's was that you shouldn't be doing your own 
> allocation decisions etc. So why do it?
> 
> Wouldn't it be much better to have a strbuf_make_room() interface that 
> just guarantees that there is enough room fo "len"? 
> 
> Otherwise, code like the above would seem to make the whole point of a 
> safer string interface rather pointless. The above code only makes sense 
> if you know how the strbuf's are internally done, so it should not exists 
> except as internal strbuf code. No?

Took a short look at strbuf.h after seeing the above code.
And I was suprised to see that all strbuf users were exposed to
the strbuf structure.
Following patch would at least make sure noone fiddle with strbuf internals.
Cut'n'paste - only for the example of it.
It simply moves strbuf declaration to the .c file where it rightfully belongs.

git did not build with this change....

	Sam


diff --git a/strbuf.c b/strbuf.c
index e33d06b..0d2d578 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -1,6 +1,14 @@
 #include "cache.h"
 #include "strbuf.h"
 
+struct strbuf {
+       int alloc;
+       int len;
+       int eof;
+       char *buf;
+};
+
+
 void strbuf_init(struct strbuf *sb) {
        sb->buf = NULL;
        sb->eof = sb->alloc = sb->len = 0;
diff --git a/strbuf.h b/strbuf.h
index 74cc012..c057be3 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -1,11 +1,6 @@
 #ifndef STRBUF_H
 #define STRBUF_H
-struct strbuf {
-       int alloc;
-       int len;
-       int eof;
-       char *buf;
-};
+struct strbuf;
 
 extern void strbuf_init(struct strbuf *);
 extern void read_line(struct strbuf *, FILE *, int);

Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c

[Pierre Habouzit]

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

On Fri, Oct 05, 2007 at 04:21:39PM +0000, Sam Ravnborg wrote:
> On Fri, Oct 05, 2007 at 08:26:44AM -0700, Linus Torvalds wrote:
> > 
> > 
> > On Fri, 5 Oct 2007, Pierre Habouzit wrote:
> > >  
> > > -	strbuf_grow(buf, len);
> > > +	/* only grow if not in place */
> > > +	if (strbuf_avail(buf) + buf->len < len)
> > > +		strbuf_grow(buf, len - buf->len);
> > 
> > Umm. This is really ugly.
> > 
> > The whole point of strbuf's was that you shouldn't be doing your own 
> > allocation decisions etc. So why do it?
> > 
> > Wouldn't it be much better to have a strbuf_make_room() interface that 
> > just guarantees that there is enough room fo "len"? 
> > 
> > Otherwise, code like the above would seem to make the whole point of a 
> > safer string interface rather pointless. The above code only makes sense 
> > if you know how the strbuf's are internally done, so it should not exists 
> > except as internal strbuf code. No?
> 
> Took a short look at strbuf.h after seeing the above code.
> And I was suprised to see that all strbuf users were exposed to
> the strbuf structure.
> Following patch would at least make sure noone fiddle with strbuf internals.
> Cut'n'paste - only for the example of it.
> It simply moves strbuf declaration to the .c file where it rightfully belongs.

  you're looking at an antiquated version, please look at the one in
current master on current next. In this one, what you can do or not do
with the struct is explained

> git did not build with this change....

  Of course it doesn't! people want to have direct access to ->buf and
->len, and it's definitely OK.

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

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

Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c

[Linus Torvalds]

On Fri, 5 Oct 2007, Sam Ravnborg wrote:
> 
> Took a short look at strbuf.h after seeing the above code.
> And I was suprised to see that all strbuf users were exposed to
> the strbuf structure.

Well, they *have* to. We want people to declare their strbufs as automatic 
or static structures, and using a opaque struct pointer is *not* an option 
(like "FILE" is doing in stdio.h).

> Following patch would at least make sure noone fiddle with strbuf internals.

No, following patch is fundamentally broken - it's not even a good 
starting point. It's bad, bad, bad.

It's also broken in another way: we want it to be really easy to use 
strbuf's as normal C strings.

Yes, many (totally idiotic and broken) interfaces think it's so important 
to "protect" their internal data structures that you have a 
"string_to_c()" helper function for that. That may be "good abstraction", 
but it's totally idiotic, because it results in horrible source code!

Tell me which is more readable:

	printf("Hello %s\n", sb->buf);

or

	printf("Hello %s\n", strbuf_to_c(sb));

and I claim that anybody who claims that the latter is "more readable" is 
full of shit, and has an agenda to push, so it's "more agenda-friendly" 
rather than readable!

So having "sb->buf" and "sb->len" be visible to users is a *good* thing. 
Otherwise you end up having to create millions of idiotic small helper 
functions, rather than just use the standard ones.

			Linus

Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c

[Sam Ravnborg]

Hi Linus.

> No, following patch is fundamentally broken - it's not even a good 
> starting point. It's bad, bad, bad.
> 
> It's also broken in another way: we want it to be really easy to use 
> strbuf's as normal C strings.
> 
> Yes, many (totally idiotic and broken) interfaces think it's so important 
> to "protect" their internal data structures that you have a 
> "string_to_c()" helper function for that. That may be "good abstraction", 
> but it's totally idiotic, because it results in horrible source code!
> 
> Tell me which is more readable:
> 
> 	printf("Hello %s\n", sb->buf);
> 
> or
> 
> 	printf("Hello %s\n", strbuf_to_c(sb));

Point taken although no sane person would name it strbuf_to_c(...).

	Sam

Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c

[Sam Ravnborg]

On Fri, Oct 05, 2007 at 06:35:17PM +0200, Pierre Habouzit wrote:
> On Fri, Oct 05, 2007 at 04:21:39PM +0000, Sam Ravnborg wrote:
> > On Fri, Oct 05, 2007 at 08:26:44AM -0700, Linus Torvalds wrote:
> > > 
> > > 
> > > On Fri, 5 Oct 2007, Pierre Habouzit wrote:
> > > >  
> > > > -	strbuf_grow(buf, len);
> > > > +	/* only grow if not in place */
> > > > +	if (strbuf_avail(buf) + buf->len < len)
> > > > +		strbuf_grow(buf, len - buf->len);
> > > 
> > > Umm. This is really ugly.
> > > 
> > > The whole point of strbuf's was that you shouldn't be doing your own 
> > > allocation decisions etc. So why do it?
> > > 
> > > Wouldn't it be much better to have a strbuf_make_room() interface that 
> > > just guarantees that there is enough room fo "len"? 
> > > 
> > > Otherwise, code like the above would seem to make the whole point of a 
> > > safer string interface rather pointless. The above code only makes sense 
> > > if you know how the strbuf's are internally done, so it should not exists 
> > > except as internal strbuf code. No?
> > 
> > Took a short look at strbuf.h after seeing the above code.
> > And I was suprised to see that all strbuf users were exposed to
> > the strbuf structure.
> > Following patch would at least make sure noone fiddle with strbuf internals.
> > Cut'n'paste - only for the example of it.
> > It simply moves strbuf declaration to the .c file where it rightfully belongs.
> 
>   you're looking at an antiquated version, please look at the one in
> current master on current next. In this one, what you can do or not do
> with the struct is explained
> 
> > git did not build with this change....
> 
>   Of course it doesn't! people want to have direct access to ->buf and
> ->len, and it's definitely OK.

Understood now - thanks for the clarification.

	Sam

Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c

[Linus Torvalds]

On Fri, 5 Oct 2007, Sam Ravnborg wrote:
> 
> Point taken although no sane person would name it strbuf_to_c(...).

I agree with the "no sane person", but the problem is that the insane 
people seem to be in no short supply.

Go look at string libraries, and they all do something like that. Or 
worse.

 - ustr: yes, it uses *exactly* what I described: "ustr_cstr()" and 
   "ustr_len()" instead of having the data/length available easily 
   (although it claims to do it for size reasons - perhaps valid in some 
   cases!)

 - libast: SPIF_CHARPTR_C(x). No, really.

 - Vstr: doesn't have a linear data representation. Needs explicit 
   flattening - although it appears to be something you're not ever 
   supposed to do - it has 200+ functions to do various magic things. 

 - Qt (QString): QString::"data()", "ascii()" or "utf8()" or something.

   At least this has the excuse of really being able to handle different 
   locales (it didn't do that originally, though!), but they end up having 
   a million helper functions exactly because you cannot use the normal 
   string routines on anything!

 - safesrtr, bstring: you just cast the pointer to "char *". Now *that* is 
   classy and safe.

So there's a few sane out there, but I actually think they are in the 
minority (Glib, others)

			Linus

Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c

[Dmitry Potapov]

On 10/5/07, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>  - Qt (QString): QString::"data()", "ascii()" or "utf8()" or something.
>
>    At least this has the excuse of really being able to handle different
>    locales (it didn't do that originally, though!), but they end up having
>    a million helper functions exactly because you cannot use the normal
>    string routines on anything!

I am afraid you cannot allow the direct access to the internal buffer of
a string if this buffer can be implicitly shared between different
instances as it is the case with QString. Because when you want to make
some modification or want to get a non-const pointer to this buffer, its
content has to be copied if the buffer is shared between a few copies.

On the other hand, I don't see what is the problem with using C string
routines with it. ::data() returns a pointer and :capacity () returns
allocated size of the buffer. ::resize() changes the size of the string.
If you need a greater allocated size, you can use ::reserve().
Or did I miss something?

Dmitry

Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c

[Linus Torvalds]

On Fri, 5 Oct 2007, Dmitry Potapov wrote:
> 
> On the other hand, I don't see what is the problem with using C string
> routines with it. ::data() returns a pointer and :capacity () returns
> allocated size of the buffer. ::resize() changes the size of the string.
> If you need a greater allocated size, you can use ::reserve().
> Or did I miss something?

No, you didn't miss anything. Except for the fact that the original point 
was that it's just a damn lot simpler and more straightforward to code 
just using "sb->buf" for the data. IOW, go back two or three emails in the 
thread.

		Linus