[PATCH] Fix handle leak in builtin-pack-objects

[PATCH] Fix handle leak in builtin-pack-objects

[Alex Riesen]

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

The opened packs seem to stay open forever.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

I'm very unsure about the solution, though: it is really horrible code
to debug...

 builtin-pack-objects.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-handle-leak-in-builtin-pack-objects.patch --]
[-- Type: text/x-diff; name=0001-Fix-handle-leak-in-builtin-pack-objects.patch, Size: 827 bytes --]

From d0fbfd05e5c1f41f58281d85d68c705c5f036e7d Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Wed, 19 Nov 2008 11:04:48 +0100
Subject: [PATCH] Fix handle leak in builtin-pack-objects

The opened packs seem to stay open forever.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 builtin-pack-objects.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 67eefa2..e68e997 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -532,6 +532,7 @@ static void write_pack_file(void)
 
 			idx_tmp_name = write_idx_file(NULL, written_list,
 						      nr_written, sha1);
+			release_pack_memory(-1, -1);
 
 			snprintf(tmpname, sizeof(tmpname), "%s-%s.pack",
 				 base_name, sha1_to_hex(sha1));
-- 
1.6.0.4.644.gb619a

Re: [PATCH] Fix handle leak in builtin-pack-objects

[Johannes Sixt]

Alex Riesen schrieb:
> The opened packs seem to stay open forever.

In my MinGW port I have the patch below that avoids that t5303 fails
because of a pack file that remains open. (Open files cannot be replaced
on Windows.) I had hoped that your patch would help, but it does not.
Something else still keeps the pack file open. Can anything be done about
that?

-- Hannes

From: Johannes Sixt <j6t@kdbg.org>
Date: Mon, 17 Nov 2008 09:25:19 +0100
Subject: [PATCH] t5303: Do not overwrite an existing pack

This test corrupts a pack file, then repacks the objects. The consequence
is that the repacked pack file has the same name as the original file
(that has been corrupted).

During its operation, git-pack-objects opens the corrupted file and keeps
it open at all times. On Windows, this is a problem because a file that is
open in any process cannot be delete or replaced, but that is what we do
in some of the test cases, and so they fail.

The work-around is to write the repacked objects to a file of a different
name, and replace the original after git-pack-objects has terminated.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 t/t5303-pack-corruption-resilience.sh |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/t/t5303-pack-corruption-resilience.sh
b/t/t5303-pack-corruption-resilience.sh
index 5132d41..41c83e3 100755
--- a/t/t5303-pack-corruption-resilience.sh
+++ b/t/t5303-pack-corruption-resilience.sh
@@ -43,8 +43,11 @@ create_new_pack() {

 do_repack() {
     pack=`printf "$blob_1\n$blob_2\n$blob_3\n" |
-          git pack-objects $@ .git/objects/pack/pack` &&
-    pack=".git/objects/pack/pack-${pack}"
+          git pack-objects $@ .git/objects/pack/packtmp` &&
+    packtmp=".git/objects/pack/packtmp-${pack}" &&
+    pack=".git/objects/pack/pack-${pack}" &&
+    mv "${packtmp}.pack" "${pack}.pack" &&
+    mv "${packtmp}.idx" "${pack}.idx"
 }

 do_corrupt_object() {
-- 
1.6.0.4.1683.g35125

Re: [PATCH] Fix handle leak in builtin-pack-objects

[Alex Riesen]

2008/11/19 Johannes Sixt <j.sixt@viscovery.net>:
> Alex Riesen schrieb:
>> The opened packs seem to stay open forever.
>
> In my MinGW port I have the patch below that avoids that t5303 fails
> because of a pack file that remains open. (Open files cannot be replaced
> on Windows.) I had hoped that your patch would help, but it does not.
> Something else still keeps the pack file open. Can anything be done about
> that?
>

Do this _and_ the other handle-leak-patch together help?
(I think the second should)

> From: Johannes Sixt <j6t@kdbg.org>
> Date: Mon, 17 Nov 2008 09:25:19 +0100

BTW, you better report such things. Even though it is typical
for this platform brokenness, leaking open files is never good.

Re: [PATCH] Fix handle leak in builtin-pack-objects

[Johannes Sixt]

Alex Riesen schrieb:
> 2008/11/19 Johannes Sixt <j.sixt@viscovery.net>:
>> Alex Riesen schrieb:
>>> The opened packs seem to stay open forever.
>> In my MinGW port I have the patch below that avoids that t5303 fails
>> because of a pack file that remains open. (Open files cannot be replaced
>> on Windows.) I had hoped that your patch would help, but it does not.
>> Something else still keeps the pack file open. Can anything be done about
>> that?
>>
> 
> Do this _and_ the other handle-leak-patch together help?
> (I think the second should)

No, that doesn't help, either. :-(

-- Hannes

Re: [PATCH] Fix handle leak in builtin-pack-objects

[Nicolas Pitre]

On Wed, 19 Nov 2008, Johannes Sixt wrote:

> Alex Riesen schrieb:
> > The opened packs seem to stay open forever.
> 
> In my MinGW port I have the patch below that avoids that t5303 fails
> because of a pack file that remains open. (Open files cannot be replaced
> on Windows.) I had hoped that your patch would help, but it does not.
> Something else still keeps the pack file open. Can anything be done about
> that?
> 
> -- Hannes
> 
> From: Johannes Sixt <j6t@kdbg.org>
> Date: Mon, 17 Nov 2008 09:25:19 +0100
> Subject: [PATCH] t5303: Do not overwrite an existing pack
> 
> This test corrupts a pack file, then repacks the objects. The consequence
> is that the repacked pack file has the same name as the original file
> (that has been corrupted).
> 
> During its operation, git-pack-objects opens the corrupted file and keeps
> it open at all times. On Windows, this is a problem because a file that is
> open in any process cannot be delete or replaced, but that is what we do
> in some of the test cases, and so they fail.
> 
> The work-around is to write the repacked objects to a file of a different
> name, and replace the original after git-pack-objects has terminated.
> 
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>

Acked-by: Nicolas Pitre <nico@cam.org>


> ---
>  t/t5303-pack-corruption-resilience.sh |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/t/t5303-pack-corruption-resilience.sh
> b/t/t5303-pack-corruption-resilience.sh
> index 5132d41..41c83e3 100755
> --- a/t/t5303-pack-corruption-resilience.sh
> +++ b/t/t5303-pack-corruption-resilience.sh
> @@ -43,8 +43,11 @@ create_new_pack() {
> 
>  do_repack() {
>      pack=`printf "$blob_1\n$blob_2\n$blob_3\n" |
> -          git pack-objects $@ .git/objects/pack/pack` &&
> -    pack=".git/objects/pack/pack-${pack}"
> +          git pack-objects $@ .git/objects/pack/packtmp` &&
> +    packtmp=".git/objects/pack/packtmp-${pack}" &&
> +    pack=".git/objects/pack/pack-${pack}" &&
> +    mv "${packtmp}.pack" "${pack}.pack" &&
> +    mv "${packtmp}.idx" "${pack}.idx"
>  }
> 
>  do_corrupt_object() {
> -- 
> 1.6.0.4.1683.g35125
> 
> 


Nicolas

Re: [PATCH] Fix handle leak in builtin-pack-objects

[Johannes Sixt]

Nicolas Pitre schrieb:
> On Wed, 19 Nov 2008, Johannes Sixt wrote:
> 
>> Alex Riesen schrieb:
>>> The opened packs seem to stay open forever.
>> In my MinGW port I have the patch below that avoids that t5303 fails
>> because of a pack file that remains open. (Open files cannot be replaced
>> on Windows.) I had hoped that your patch would help, but it does not.
>> Something else still keeps the pack file open. Can anything be done about
>> that?
>>
>> -- Hannes
>>
>> From: Johannes Sixt <j6t@kdbg.org>
>> Date: Mon, 17 Nov 2008 09:25:19 +0100
>> Subject: [PATCH] t5303: Do not overwrite an existing pack
>>
...
> Acked-by: Nicolas Pitre <nico@cam.org>

Thanks, but I should have mentioned that at this time this patch was just
meant for exposition, not inclusion. [It's just one of many, many patches
needed to make the test suite pass on MinGW.]

I'd prefer a solution to the problem that the pack file remains open. Do
you have an idea where git-pack-objects keeps the pack file open, even
with Alex's two patches applied?

-- Hannes

Re: [PATCH] Fix handle leak in builtin-pack-objects

[Nicolas Pitre]

On Wed, 19 Nov 2008, Johannes Sixt wrote:

> Nicolas Pitre schrieb:
> > On Wed, 19 Nov 2008, Johannes Sixt wrote:
> > 
> >> Alex Riesen schrieb:
> >>> The opened packs seem to stay open forever.
> >> In my MinGW port I have the patch below that avoids that t5303 fails
> >> because of a pack file that remains open. (Open files cannot be replaced
> >> on Windows.) I had hoped that your patch would help, but it does not.
> >> Something else still keeps the pack file open. Can anything be done about
> >> that?
> >>
> >> -- Hannes
> >>
> >> From: Johannes Sixt <j6t@kdbg.org>
> >> Date: Mon, 17 Nov 2008 09:25:19 +0100
> >> Subject: [PATCH] t5303: Do not overwrite an existing pack
> >>
> ...
> > Acked-by: Nicolas Pitre <nico@cam.org>
> 
> Thanks, but I should have mentioned that at this time this patch was just
> meant for exposition, not inclusion.

Well, I'd include it right away since it is fundamentally the right 
thing to do.

> I'd prefer a solution to the problem that the pack file remains open. Do
> you have an idea where git-pack-objects keeps the pack file open, even
> with Alex's two patches applied?

That's not the issue.

The test is using pack-objects to overwrite a pack file, but to do so 
pack-objects has to open and read from the pack file about to be 
overwritten.  This is just wrong even if it happens to work by luck on 
Linux.

It is on purpose that the pack files are kept open.  This is to minimize 
the number of open() and mmap()/read() operations between successive 
object reads.  Those are not leaked file handles since they get closed 
when new packs are opened and the number of concurrent opened packs 
reaches a certain limit.


Nicolas

Re: [PATCH] Fix handle leak in builtin-pack-objects

[Alex Riesen]

2008/11/19 Nicolas Pitre <nico@cam.org>:
> On Wed, 19 Nov 2008, Johannes Sixt wrote:
>> The work-around is to write the repacked objects to a file of a different
>> name, and replace the original after git-pack-objects has terminated.
>>
>> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
>
> Acked-by: Nicolas Pitre <nico@cam.org>
>

Are you sure? Will it work in a real repository? Were noone does
rename the previous pack files into packtmp-something?

Re: [PATCH] Fix handle leak in builtin-pack-objects

[Johannes Sixt]

Alex Riesen schrieb:
> 2008/11/19 Nicolas Pitre <nico@cam.org>:
>> On Wed, 19 Nov 2008, Johannes Sixt wrote:
>>> The work-around is to write the repacked objects to a file of a different
>>> name, and replace the original after git-pack-objects has terminated.
>>>
>>> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
>> Acked-by: Nicolas Pitre <nico@cam.org>
> 
> Are you sure? Will it work in a real repository? Were noone does
> rename the previous pack files into packtmp-something?

Oh, the patch only works around the failure in the test case. In a real
repository there is usually no problem because the destination pack file
does not exist.

The unusual case is where you do this:

 $ git rev-list -10 HEAD | git pack-objects foobar

twice in a row: In this case the second invocation fails on Windows
because the destination pack file already exists *and* is open. But not
even git-repack does this even if it is called twice. OTOH, the test case
*does* exactly this.

-- Hannes

Re: [PATCH] Fix handle leak in builtin-pack-objects

[Alex Riesen]

2008/11/19 Johannes Sixt <j.sixt@viscovery.net>:
> Alex Riesen schrieb:
>> 2008/11/19 Nicolas Pitre <nico@cam.org>:
>>> On Wed, 19 Nov 2008, Johannes Sixt wrote:
>>>> The work-around is to write the repacked objects to a file of a different
>>>> name, and replace the original after git-pack-objects has terminated.
>>>>
>>>> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
>>> Acked-by: Nicolas Pitre <nico@cam.org>
>>
>> Are you sure? Will it work in a real repository? Were noone does
>> rename the previous pack files into packtmp-something?
>
> Oh, the patch only works around the failure in the test case. In a real
> repository there is usually no problem because the destination pack file
> does not exist.
>
> The unusual case is where you do this:
>
>  $ git rev-list -10 HEAD | git pack-objects foobar
>
> twice in a row: In this case the second invocation fails on Windows
> because the destination pack file already exists *and* is open. But not
> even git-repack does this even if it is called twice. OTOH, the test case
> *does* exactly this.
>

Still bad...

BTW, the patch _does_ fix the "unusual case" for me. IOW, I plainly copied
your rev-list|pack-objects into command line. As expected, it fails for Junio's
master (after the second run) and works with the patch:

  $ git rev-list -10 HEAD | ./git pack-objects .git/objects/pack/foobar
  Counting objects: 10, done.
  Compressing objects: 100% (9/9), done.
  82864ec14cd06e6089543a1419762e4cd40f7988
  Writing objects: 100% (10/10), done.
  Total 10 (delta 1), reused 10 (delta 1)

  $ git rev-list -10 HEAD | ./git pack-objects .git/objects/pack/foobar
  Counting objects: 10, done.
  Compressing objects: 100% (9/9), done.
  fatal: unable to rename temporary pack file: Permission denied

  $ git cherry-pick fix-fd-leak
  Finished one cherry-pick.
  [master]: created e629465: "Fix handle leak in builtin-pack-objects"
   1 files changed, 1 insertions(+), 0 deletions(-)

  $ git rev-list -10 HEAD | ./git pack-objects .git/objects/pack/foobar
  Counting objects: 10, done.
  Compressing objects: 100% (9/9), done.
  0e43d919a2a8336fd740d8d9b8f9f78d49e855e5
  Writing objects: 100% (10/10), done.
  Total 10 (delta 1), reused 9 (delta 1)

  $ git rev-list -10 HEAD | ./git pack-objects .git/objects/pack/foobar
  Counting objects: 10, done.
  Compressing objects: 100% (9/9), done.
  0e43d919a2a8336fd740d8d9b8f9f78d49e855e5
  Writing objects: 100% (10/10), done.
  Total 10 (delta 1), reused 10 (delta 1)

  $ git rev-list -10 HEAD | ./git pack-objects .git/objects/pack/foobar
  Counting objects: 10, done.
  Compressing objects: 100% (9/9), done.
  0e43d919a2a8336fd740d8d9b8f9f78d49e855e5
  Writing objects: 100% (10/10), done.
  Total 10 (delta 1), reused 10 (delta 1)

Re: [PATCH] Fix handle leak in builtin-pack-objects

[Nicolas Pitre]

On Wed, 19 Nov 2008, Johannes Sixt wrote:

> Alex Riesen schrieb:
> > 2008/11/19 Nicolas Pitre <nico@cam.org>:
> >> On Wed, 19 Nov 2008, Johannes Sixt wrote:
> >>> The work-around is to write the repacked objects to a file of a different
> >>> name, and replace the original after git-pack-objects has terminated.
> >>>
> >>> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> >> Acked-by: Nicolas Pitre <nico@cam.org>
> > 
> > Are you sure? Will it work in a real repository? Were noone does
> > rename the previous pack files into packtmp-something?
> 
> Oh, the patch only works around the failure in the test case. In a real
> repository there is usually no problem because the destination pack file
> does not exist.
> 
> The unusual case is where you do this:
> 
>  $ git rev-list -10 HEAD | git pack-objects foobar
> 
> twice in a row: In this case the second invocation fails on Windows
> because the destination pack file already exists *and* is open. But not
> even git-repack does this even if it is called twice. OTOH, the test case
> *does* exactly this.

OK.... Well, despite my earlier assertion, I think the above should be a 
valid operation.

I'm looking at it now.  I'm therefore revoking my earlier ACK as well 
(better keep that test case alive).


Nicolas

Re: [PATCH] Fix handle leak in builtin-pack-objects

[Johannes Sixt]

Nicolas Pitre schrieb:
> On Wed, 19 Nov 2008, Johannes Sixt wrote:
>> The unusual case is where you do this:
>>
>>  $ git rev-list -10 HEAD | git pack-objects foobar
>>
>> twice in a row: In this case the second invocation fails on Windows
>> because the destination pack file already exists *and* is open. But not
>> even git-repack does this even if it is called twice. OTOH, the test case
>> *does* exactly this.
> 
> OK.... Well, despite my earlier assertion, I think the above should be a 
> valid operation.
> 
> I'm looking at it now.  I'm therefore revoking my earlier ACK as well 
> (better keep that test case alive).

Hold on a moment: When I tested the above sequence, I was fooled by a flaw
in mingw_rename() (it doesn't replace read-only files). With that fixed,
it works as expected in repeated invocations (note that foobar is outside
the .git/objects/pack directory).

If I use .git/objects/pack/foobar instead, then I get the failures on
Windows, and I won't argue that this should be "fixed". ;)

-- Hannes

[PATCH] compat/mingw.c: Teach mingw_rename() to replace read-only files

[Johannes Sixt]

From: Johannes Sixt <j6t@kdbg.org>

On POSIX, rename() can replace files that are not writable. On Windows,
however, read-only files cannot be replaced without additional efforts:
We have to make the destination writable first.

Since the situations where the destination is read-only are rare, we do not
make the destination writable on every invocation, but only if the first
try to rename a file failed with an "access denied" error.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Johannes Sixt schrieb:
> Nicolas Pitre schrieb:
>> On Wed, 19 Nov 2008, Johannes Sixt wrote:
>>> The unusual case is where you do this:
>>>
>>>  $ git rev-list -10 HEAD | git pack-objects foobar
>>>
>>> twice in a row: In this case the second invocation fails on Windows
>>> because the destination pack file already exists *and* is open. But not
>>> even git-repack does this even if it is called twice. OTOH, the test case
>>> *does* exactly this.
>> OK.... Well, despite my earlier assertion, I think the above should be a 
>> valid operation.

Would you please clarify which operation you exactly mean? As is written
above, or with .git/objects/pack/foobar instead like in the test case?

>> I'm looking at it now.  I'm therefore revoking my earlier ACK as well 
>> (better keep that test case alive).
> 
> Hold on a moment: When I tested the above sequence, I was fooled by a flaw
> in mingw_rename() (it doesn't replace read-only files). With that fixed...

Here is this fix.

-- Hannes

 compat/mingw.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index b534a8a..3dbe6a7 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -819,6 +819,8 @@ int mingw_connect(int sockfd, struct sockaddr *sa, size_t sz)
 #undef rename
 int mingw_rename(const char *pold, const char *pnew)
 {
+	DWORD attrs;
+
 	/*
 	 * Try native rename() first to get errno right.
 	 * It is based on MoveFile(), which cannot overwrite existing files.
@@ -830,12 +832,19 @@ int mingw_rename(const char *pold, const char *pnew)
 	if (MoveFileEx(pold, pnew, MOVEFILE_REPLACE_EXISTING))
 		return 0;
 	/* TODO: translate more errors */
-	if (GetLastError() == ERROR_ACCESS_DENIED) {
-		DWORD attrs = GetFileAttributes(pnew);
-		if (attrs != INVALID_FILE_ATTRIBUTES && (attrs & FILE_ATTRIBUTE_DIRECTORY)) {
+	if (GetLastError() == ERROR_ACCESS_DENIED &&
+	    (attrs = GetFileAttributes(pnew)) != INVALID_FILE_ATTRIBUTES) {
+		if (attrs & FILE_ATTRIBUTE_DIRECTORY) {
 			errno = EISDIR;
 			return -1;
 		}
+		if ((attrs & FILE_ATTRIBUTE_READONLY) &&
+		    SetFileAttributes(pnew, attrs & ~FILE_ATTRIBUTE_READONLY)) {
+			if (MoveFileEx(pold, pnew, MOVEFILE_REPLACE_EXISTING))
+				return 0;
+			/* revert file attributes on failure */
+			SetFileAttributes(pnew, attrs);
+		}
 	}
 	errno = EACCES;
 	return -1;
-- 
1.6.0.4.1694.g07ac

Re: [PATCH] Fix handle leak in builtin-pack-objects

[Alex Riesen]

2008/11/19 Nicolas Pitre <nico@cam.org>:
> On Wed, 19 Nov 2008, Johannes Sixt wrote:
>> Alex Riesen schrieb:
>> > 2008/11/19 Nicolas Pitre <nico@cam.org>:
>> >> On Wed, 19 Nov 2008, Johannes Sixt wrote:
>> >>> The work-around is to write the repacked objects to a file of a different
>> >>> name, and replace the original after git-pack-objects has terminated.
>> >>>
>> >>> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
>> >> Acked-by: Nicolas Pitre <nico@cam.org>
>> >
>> > Are you sure? Will it work in a real repository? Were noone does
>> > rename the previous pack files into packtmp-something?
>>
>> Oh, the patch only works around the failure in the test case. In a real
>> repository there is usually no problem because the destination pack file
>> does not exist.
>>
>> The unusual case is where you do this:
>>
>>  $ git rev-list -10 HEAD | git pack-objects foobar
>>
>> twice in a row: In this case the second invocation fails on Windows
>> because the destination pack file already exists *and* is open. But not
>> even git-repack does this even if it is called twice. OTOH, the test case
>> *does* exactly this.
>
> OK.... Well, despite my earlier assertion, I think the above should be a
> valid operation.
>
> I'm looking at it now.  I'm therefore revoking my earlier ACK as well
> (better keep that test case alive).
>

Any news here?

Re: [PATCH] Fix handle leak in builtin-pack-objects

[Nicolas Pitre]

On Wed, 26 Nov 2008, Alex Riesen wrote:

> 2008/11/19 Nicolas Pitre <nico@cam.org>:
> > On Wed, 19 Nov 2008, Johannes Sixt wrote:
> >> Alex Riesen schrieb:
> >> > 2008/11/19 Nicolas Pitre <nico@cam.org>:
> >> >> On Wed, 19 Nov 2008, Johannes Sixt wrote:
> >> >>> The work-around is to write the repacked objects to a file of a different
> >> >>> name, and replace the original after git-pack-objects has terminated.
> >> >>>
> >> >>> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> >> >> Acked-by: Nicolas Pitre <nico@cam.org>
> >> >
> >> > Are you sure? Will it work in a real repository? Were noone does
> >> > rename the previous pack files into packtmp-something?
> >>
> >> Oh, the patch only works around the failure in the test case. In a real
> >> repository there is usually no problem because the destination pack file
> >> does not exist.
> >>
> >> The unusual case is where you do this:
> >>
> >>  $ git rev-list -10 HEAD | git pack-objects foobar
> >>
> >> twice in a row: In this case the second invocation fails on Windows
> >> because the destination pack file already exists *and* is open. But not
> >> even git-repack does this even if it is called twice. OTOH, the test case
> >> *does* exactly this.
> >
> > OK.... Well, despite my earlier assertion, I think the above should be a
> > valid operation.
> >
> > I'm looking at it now.  I'm therefore revoking my earlier ACK as well
> > (better keep that test case alive).
> >
> 
> Any news here?

Yes: my hard disk crashed a couple hours after I mentioned this, so most 
of my time has been spent on recovery since then.  I'll come back to it 
eventually.


Nicolas

Re: [PATCH] Fix handle leak in builtin-pack-objects

[Alex Riesen]

Nicolas Pitre, Wed, Nov 26, 2008 15:33:51 +0100:
> On Wed, 26 Nov 2008, Alex Riesen wrote:
> > 2008/11/19 Nicolas Pitre <nico@cam.org>:
> > > I'm looking at it now.  I'm therefore revoking my earlier ACK as well
> > > (better keep that test case alive).
> > >
> > 
> > Any news here?
> 
> Yes: my hard disk crashed a couple hours after I mentioned this, so most 
> of my time has been spent on recovery since then.  I'll come back to it 
> eventually.

Heh... Good luck!

[PATCH] make sure packs to be replaced are closed beforehand

[Nicolas Pitre]

Especially on Windows where an opened file cannot be replaced, make
sure pack-objects always close packs it is about to replace. Even on
non Windows systems, this could save potential bad results if ever
objects were to be read from the new pack file using offset from the old
index.

This should fix t5303 on Windows.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---

On Wed, 26 Nov 2008, Nicolas Pitre wrote:
> On Wed, 26 Nov 2008, Alex Riesen wrote:
> > 2008/11/19 Nicolas Pitre <nico@cam.org>:
> > > On Wed, 19 Nov 2008, Johannes Sixt wrote:
> > >> Oh, the patch only works around the failure in the test case. In a real
> > >> repository there is usually no problem because the destination pack file
> > >> does not exist.
> > >>
> > >> The unusual case is where you do this:
> > >>
> > >>  $ git rev-list -10 HEAD | git pack-objects foobar
> > >>
> > >> twice in a row: In this case the second invocation fails on Windows
> > >> because the destination pack file already exists *and* is open. But not
> > >> even git-repack does this even if it is called twice. OTOH, the test case
> > >> *does* exactly this.
> > >
> > > OK.... Well, despite my earlier assertion, I think the above should be a
> > > valid operation.
> > >
> > > I'm looking at it now.  I'm therefore revoking my earlier ACK as well
> > > (better keep that test case alive).
> > >
> > 
> > Any news here?
> 
> Yes: my hard disk crashed a couple hours after I mentioned this, so most 
> of my time has been spent on recovery since then.  I'll come back to it 
> eventually.

OK, here it is at last.  Please confirm it works on Windows before Junio 
merges it.

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 67eefa2..cedef52 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -535,6 +535,7 @@ static void write_pack_file(void)
 
 			snprintf(tmpname, sizeof(tmpname), "%s-%s.pack",
 				 base_name, sha1_to_hex(sha1));
+			free_pack_by_name(tmpname);
 			if (adjust_perm(pack_tmp_name, mode))
 				die("unable to make temporary pack file readable: %s",
 				    strerror(errno));
diff --git a/cache.h b/cache.h
index f15b3fc..231c06d 100644
--- a/cache.h
+++ b/cache.h
@@ -820,6 +820,7 @@ extern int open_pack_index(struct packed_git *);
 extern unsigned char* use_pack(struct packed_git *, struct pack_window **, off_t, unsigned int *);
 extern void close_pack_windows(struct packed_git *);
 extern void unuse_pack(struct pack_window **);
+extern void free_pack_by_name(const char *);
 extern struct packed_git *add_packed_git(const char *, int, int);
 extern const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t);
 extern off_t nth_packed_object_offset(const struct packed_git *, uint32_t);
diff --git a/sha1_file.c b/sha1_file.c
index 6c0e251..0e021c5 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -673,6 +673,37 @@ void unuse_pack(struct pack_window **w_cursor)
 }
 
 /*
+ * This is used by git-repack in case a newly created pack happens to
+ * contain the same set of objects as an existing one.  In that case
+ * the resulting file might be different even if its name would be the
+ * same.  It is best to close any reference to the old pack before it is
+ * replaced on disk.  Of course no index pointers nor windows for given pack
+ * must subsist at this point.  If ever objects from this pack are requested
+ * again, the new version of the pack will be reinitialized through
+ * reprepare_packed_git().
+ */
+void free_pack_by_name(const char *pack_name)
+{
+	struct packed_git *p, **pp = &packed_git;
+
+	while (*pp) {
+		p = *pp;
+		if (strcmp(pack_name, p->pack_name) == 0) {
+			close_pack_windows(p);
+			if (p->pack_fd != -1)
+				close(p->pack_fd);
+			if (p->index_data)
+				munmap((void *)p->index_data, p->index_size);
+			free(p->bad_object_sha1);
+			*pp = p->next;
+			free(p);
+			return;
+		}
+		pp = &p->next;
+	}
+}
+
+/*
  * Do not call this directly as this leaks p->pack_fd on error return;
  * call open_packed_git() instead.
  */

Re: [PATCH] make sure packs to be replaced are closed beforehand

[Alex Riesen]

Nicolas Pitre, Tue, Dec 09, 2008 20:26:52 +0100:
> Especially on Windows where an opened file cannot be replaced, make
> sure pack-objects always close packs it is about to replace. Even on
> non Windows systems, this could save potential bad results if ever
> objects were to be read from the new pack file using offset from the old
> index.
> 
> This should fix t5303 on Windows.
> 
> Signed-off-by: Nicolas Pitre <nico@cam.org>
> ---
...
> OK, here it is at last.  Please confirm it works on Windows before Junio 
> merges it.
> 

Will do in about 16 hours.

Re: [PATCH] make sure packs to be replaced are closed beforehand

[Johannes Sixt]

Nicolas Pitre schrieb:
> Especially on Windows where an opened file cannot be replaced, make
> sure pack-objects always close packs it is about to replace. Even on
> non Windows systems, this could save potential bad results if ever
> objects were to be read from the new pack file using offset from the old
> index.
> 
> This should fix t5303 on Windows.
...
> OK, here it is at last.  Please confirm it works on Windows before Junio 
> merges it.

I can confirm that this patch fixes t5303 on Windows (MinGW).

-- Hannes

Re: [PATCH] make sure packs to be replaced are closed beforehand

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Nicolas Pitre schrieb:
>> Especially on Windows where an opened file cannot be replaced, make
>> sure pack-objects always close packs it is about to replace. Even on
>> non Windows systems, this could save potential bad results if ever
>> objects were to be read from the new pack file using offset from the old
>> index.
>> 
>> This should fix t5303 on Windows.
> ...
>> OK, here it is at last.  Please confirm it works on Windows before Junio 
>> merges it.
>
> I can confirm that this patch fixes t5303 on Windows (MinGW).

Thanks; it is a bit too late for tonight, but it will appear in tomorrow's
'master'.

Re: [PATCH] make sure packs to be replaced are closed beforehand

[Alex Riesen]

2008/12/10 Johannes Sixt <j.sixt@viscovery.net>:
> Nicolas Pitre schrieb:
>> Especially on Windows where an opened file cannot be replaced, make
>> sure pack-objects always close packs it is about to replace. Even on
>> non Windows systems, this could save potential bad results if ever
>> objects were to be read from the new pack file using offset from the old
>> index.
>>
>> This should fix t5303 on Windows.
> ...
>> OK, here it is at last.  Please confirm it works on Windows before Junio
>> merges it.
>
> I can confirm that this patch fixes t5303 on Windows (MinGW).
>

And it does that for me, FWIW.

Re: [PATCH] Fix handle leak in builtin-pack-objects

[Nicolas Pitre]

On Wed, 19 Nov 2008, Alex Riesen wrote:

> The opened packs seem to stay open forever.
> 
> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
> ---
> 
> I'm very unsure about the solution, though: it is really horrible code
> to debug...

What is the actual problem?  Pack windows are left open on purpose.


Nicolas

Re: [PATCH] Fix handle leak in builtin-pack-objects

[Alex Riesen]

2008/11/19 Nicolas Pitre <nico@cam.org>:
> On Wed, 19 Nov 2008, Alex Riesen wrote:
>
>> The opened packs seem to stay open forever.
>>
>> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
>> ---
>>
>> I'm very unsure about the solution, though: it is really horrible code
>> to debug...
>
> What is the actual problem?  Pack windows are left open on purpose.
>

* expecting success: do_repack &&
     git prune-packed -v &&
     git verify-pack ${pack}.pack &&
     git cat-file blob $blob_1 > /dev/null &&
     git cat-file blob $blob_2 > /dev/null &&
     git cat-file blob $blob_3 > /dev/null
Counting objects: 3, done.
error: unknown object type 0 at offset 2032 in
.git/objects/pack/pack-8d1e04fc992cfbdb3ab72cf7abbf08cce8b1900.pack
Compressing objects: 100% (2/2), done.
fatal: unable to rename temporary pack file
.git/objects/pack/tmp_pack_s9Gijm->.git/objects/pack/pack-b8d1e04fc992cfbdbab72cf7abbf08cce8b1900.pack:
Permission denied
* FAIL 10: ... and then a repack "clears" the corruption
        do_repack &&
             git prune-packed -v &&
             git verify-pack ${pack}.pack &&
             git cat-file blob $blob_1 > /dev/null &&
             git cat-file blob $blob_2 > /dev/null &&
             git cat-file blob $blob_3 > /dev/null