[PATCH] git-apply: Do not free the wrong buffer when we convert the data for writeout

[PATCH] git-apply: Do not free the wrong buffer when we convert the data for writeout

[Junio C Hamano]

When we write out the result of patch application, we sometimes
need to munge the data (e.g. under core.autocrlf).  After doing
so, what we should free is the temporary buffer that holds the
converted data returned from convert_to_working_tree(), not the
original one.

This patch also moves the call to open() up in the function, as
the caller expects us to fail cheaply if leading directories
need to be created (and then the caller creates them and calls
us again).  For that calling pattern, attempting conversion
before opening the file adds unnecessary overhead.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

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

  > In "try_create_file()", we do:
  >
  > 	...
  >         if (convert_to_working_tree(path, &nbuf, &nsize)) {
  >                 free((char *) buf);
  >                 buf = nbuf;
  >                 size = nsize;
  >         }
  > 	...
  >
  > but the thing is, the *caller* still uses the old "buf/nsize", so when you 
  > free it, the caller will now use the free'd data structure, and if it gets 
  > re-used by - for example - the zlib deflate() buffers, you'll get a 
  > corrupt object (if it gets re-used *before*, you'll get the *wrong* 
  > object!). Exactly Alexander's patterns.
  >
  > Alexander - sorry for all the trouble, this was definitely our bad.

  I concur.  Sorry for this, Alexander.

  git-apply in general is quite leaky (e.g. it never frees a
  finished patch, and freeing a patch is quite difficult as some
  strings like filenames are shared without refcounting), but
  this part deals with a large amount of data and I would rather
  not add a new one.

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

diff --git a/builtin-apply.c b/builtin-apply.c
index dfa1716..27a182b 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2355,7 +2355,7 @@ static void add_index_file(const char *path, unsigned mode, void *buf, unsigned
 
 static int try_create_file(const char *path, unsigned int mode, const char *buf, unsigned long size)
 {
-	int fd;
+	int fd, converted;
 	char *nbuf;
 	unsigned long nsize;
 
@@ -2364,17 +2364,18 @@ static int try_create_file(const char *path, unsigned int mode, const char *buf,
 		 * terminated.
 		 */
 		return symlink(buf, path);
+
+	fd = open(path, O_CREAT | O_EXCL | O_WRONLY, (mode & 0100) ? 0777 : 0666);
+	if (fd < 0)
+		return -1;
+
 	nsize = size;
 	nbuf = (char *) buf;
-	if (convert_to_working_tree(path, &nbuf, &nsize)) {
-		free((char *) buf);
+	converted = convert_to_working_tree(path, &nbuf, &nsize);
+	if (converted) {
 		buf = nbuf;
 		size = nsize;
 	}
-
-	fd = open(path, O_CREAT | O_EXCL | O_WRONLY, (mode & 0100) ? 0777 : 0666);
-	if (fd < 0)
-		return -1;
 	while (size) {
 		int written = xwrite(fd, buf, size);
 		if (written < 0)
@@ -2386,6 +2387,8 @@ static int try_create_file(const char *path, unsigned int mode, const char *buf,
 	}
 	if (close(fd) < 0)
 		die("closing file %s: %s", path, strerror(errno));
+	if (converted)
+		free(nbuf);
 	return 0;
 }
 

Re: [PATCH] git-apply: Do not free the wrong buffer when we convert the data for writeout

[Linus Torvalds]

On Thu, 22 Mar 2007, Junio C Hamano wrote:
> 
> This patch also moves the call to open() up in the function, as
> the caller expects us to fail cheaply if leading directories
> need to be created (and then the caller creates them and calls
> us again).  For that calling pattern, attempting conversion
> before opening the file adds unnecessary overhead.
> 
> Signed-off-by: Junio C Hamano <junkio@cox.net>

Ack, looks good.

		Linus

Re: [PATCH] git-apply: Do not free the wrong buffer when we convert the data for writeout

[Alexander Litvinov]

В сообщении от Friday 23 March 2007 02:55 Linus Torvalds написал(a):
> On Thu, 22 Mar 2007, Junio C Hamano wrote:
> > This patch also moves the call to open() up in the function, as
> > the caller expects us to fail cheaply if leading directories
> > need to be created (and then the caller creates them and calls
> > us again).  For that calling pattern, attempting conversion
> > before opening the file adds unnecessary overhead.

I have applied this patch ontop of next (d06644b) and it also fix by repo 
breakage. 

Thanks for help !
Alexander Litvinov.

Re: [PATCH] git-apply: Do not free the wrong buffer when we convert the data for writeout

[皐月]

iPhoneから送信