patchFix in-place editing in crlf_to_git and ident_to_git.

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(-)

* 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(-)

Pierre Habouzit schrieb:

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

This fixes it for me.  Thanks!!

Bernt

On Fri, 5 Oct 2007, Pierre Habouzit wrote:

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

On Fri, Oct 05, 2007 at 03:26:44PM +0000, Linus Torvalds wrote:

  I agree.

  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...

  Right, that would do the same btw ;)

  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]

On Fri, Oct 05, 2007 at 08:26:44AM -0700, Linus Torvalds 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. 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

On Fri, Oct 05, 2007 at 04:21:39PM +0000, Sam Ravnborg wrote:

  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

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

On Fri, Oct 05, 2007 at 06:35:17PM +0200, Pierre Habouzit wrote:

Understood now - thanks for the clarification.

	Sam

On Fri, 5 Oct 2007, Sam Ravnborg wrote:

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).

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

Hi Linus.

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

	Sam

On Fri, 5 Oct 2007, Sam Ravnborg wrote:

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

On 10/5/07, Linus Torvalds <torvalds@linux-foundation.org> wrote:

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

On Fri, 5 Oct 2007, Dmitry Potapov wrote:

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

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(-)

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(-)

Hi,

On Fri, 5 Oct 2007, Pierre Habouzit wrote:

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

Ciao, Dscho

On Fri, Oct 05, 2007 at 08:30:45AM +0000, Johannes Schindelin wrote:

  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.