git-upload-pack: the timeout gets corrupted?!

DISCUSSION H. Peter Anvin 3 participants 10 messages Jun 15, 2016

Share & tools

H. Peter Anvin

author

2016-06-15 22:42 UTC #0 lore raw

git-1.5.0.3-1.i386 rpm from Junio's repository on kernel.org:

Since we got the new git server on kernel.org, we are having a problem with git-upload-pack processes getting reparented to init, and then sitting there forever. Going in with gdb, it appears the "timeout" variable gets overwritten:

(gdb) p timeout
$1 = 608471321

... which should have been 600.

The process spends effectively forever waiting in on the fflush() in show_commit() (in upload-pack.c); /proc/*/fd shows it is trying to write to a pipe, but I'm not sure what is at the other end of that same pipe.

-hpa

H. Peter Anvin

author

2016-06-15 22:42 UTC #1 lore raw

Quoted 11 lines · H. Peter Anvin

git-1.5.0.3-1.i386 rpm from Junio's repository on kernel.org:
Since we got the new git server on kernel.org, we are having a problem with git-upload-pack processes getting reparented to init, and then sitting there forever. Going in with gdb, it appears the "timeout" variable gets overwritten:
(gdb) p timeout
$1 = 608471321
... which should have been 600.
The process spends effectively forever waiting in on the fflush() in show_commit() (in upload-pack.c); /proc/*/fd shows it is trying to write to a pipe, but I'm not sure what is at the other end of that same pipe.

Another observation:

The value is always 608471321 (0x24448919) across all stuck processes that I've examined. This is neither a recent time_t value (it would be Thu Apr 13 11:48:41 UTC 1989) nor is it a valid pointer value in any of the processes I've looked at.

-hpa

H. Peter Anvin

author

2016-06-15 22:42 UTC #2 lore raw

Quoted 3 lines · H. Peter Anvin

Another observation:
The value is always 608471321 (0x24448919) across all stuck processes that I've examined. This is neither a recent time_t value (it would be Thu Apr 13 11:48:41 UTC 1989) nor is it a valid pointer value in any of the processes I've looked at.

Nevermind! I just noticed that git-debuginfo RPM isn't from the same build as the git-core RPM, so anything obtained with gdb is baloney...

-hpa

Junio C Hamano

2016-06-15 22:42 UTC #3 lore raw

In reply to H. Peter Anvin

Quoted 7 lines · H. Peter Anvin

... Going in with gdb, it appears the "timeout"
variable gets overwritten:
(gdb) p timeout
$1 = 608471321
... which should have been 600.

I do not see offhand what can cause this. The only new code since 1.4.4 series is "shallow clone" stuff,...

Quoted 4 lines

The process spends effectively forever waiting in on the fflush() in show_commit() (in upload-pack.c); /proc/*/fd shows it is trying to write to a pipe, but I'm not sure what is at the other end of that same pipe.

The process forks and the one that runs show_commit() is running rev-list internally while the other end is a pack-objects that reads from it and sends its output back to the client.

H. Peter Anvin

author

2016-06-15 22:42 UTC #4 lore raw

Quoted 13 lines · Junio C Hamano

Quoted 7 lines · H. Peter Anvin
... Going in with gdb, it appears the "timeout"
variable gets overwritten:
(gdb) p timeout
$1 = 608471321
... which should have been 600.
I do not see offhand what can cause this. The only new code since 1.4.4 series is "shallow clone" stuff,...

Quoted 9 lines

Quoted 4 lines
The process spends effectively forever waiting in on the fflush() in show_commit() (in upload-pack.c); /proc/*/fd shows it is trying to write to a pipe, but I'm not sure what is at the other end of that same pipe.
The process forks and the one that runs show_commit() is running rev-list internally while the other end is a pack-objects that reads from it and sends its output back to the client.

Yup; the pack-objects process has apparently died.

-hpa

H. Peter Anvin

author

2016-06-15 22:42 UTC #5 lore raw

In reply to Junio C Hamano

Quoted 10 lines · Junio C Hamano

Quoted 5 lines
The process spends effectively forever waiting in on the fflush() in show_commit() (in upload-pack.c); /proc/*/fd shows it is trying to write to a pipe, but I'm not sure what is at the other end of that same pipe.
The process forks and the one that runs show_commit() is running rev-list internally while the other end is a pack-objects that reads from it and sends its output back to the client.

Now, given that the fact that the git-pack-object process has already died, normally one would expect the write() to get SIGPIPE which would kill the process. Does git-upload-pack not close the read end of the pipe in the writer? From the looks of the fd directory, I would say it does not.

-hpa

Johannes Schindelin

2016-06-15 22:42 UTC #6 lore raw

Hi,

Quoted 12 lines · H. Peter Anvin

Quoted 10 lines · Junio C Hamano
Quoted 5 lines
The process spends effectively forever waiting in on the fflush() in show_commit() (in upload-pack.c); /proc/*/fd shows it is trying to write to a pipe, but I'm not sure what is at the other end of that same pipe.
The process forks and the one that runs show_commit() is running rev-list internally while the other end is a pack-objects that reads from it and sends its output back to the client.

Quoted 4 lines

Now, given that the fact that the git-pack-object process has already died, normally one would expect the write() to get SIGPIPE which would kill the process. Does git-upload-pack not close the read end of the pipe in the writer? From the looks of the fd directory, I would say it does not.

Something like this (totally untested):

 upload-pack.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

upload-pack.c +6 -0

index 498bf50..bafd90f 100644
@@ -119,6 +119,8 @@ static void create_pack_file(void)
 		int i;
 		struct rev_info revs;
 
+		close(0);
+
 		pack_pipe = fdopen(lp_pipe[1], "w");
 
 		if (create_full_pack)

@@ -167,6 +169,10 @@ static void create_pack_file(void)
 		const char *argv[10];
 		int i = 0;
 
+		close(0);
+		close(1);
+		close(2);
+
 		dup2(lp_pipe[0], 0);
 		dup2(pu_pipe[1], 1);
 		dup2(pe_pipe[1], 2);

Ciao,
Dscho

H. Peter Anvin

author

2016-06-15 22:42 UTC #7 lore raw

Quoted 11 lines · Johannes Schindelin

Something like this (totally untested):
 upload-pack.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
upload-pack.c +0 -0
index 498bf50..bafd90f 100644

upload-pack.c static void create_pack_file(void) · line 126

Quoted 8 lines

upload-pack.c +2 -0

@@ -119,6 +119,8 @@ static void create_pack_file(void)
 		int i;
 		struct rev_info revs;
 
+		close(0);
+
 		pack_pipe = fdopen(lp_pipe[1], "w");
 
 		if (create_full_pack)

Shouldn't that be close(lp_pipe[0]);?

upload-pack.c static void create_pack_file(void) · line 178

Quoted 12 lines

upload-pack.c +4 -0

@@ -167,6 +169,10 @@ static void create_pack_file(void)
 		const char *argv[10];
 		int i = 0;
 
+		close(0);
+		close(1);
+		close(2);
+
 		dup2(lp_pipe[0], 0);
 		dup2(pu_pipe[1], 1);
 		dup2(pe_pipe[1], 2);

Those close()'s are redundant with the dup2's...

-hpa

Johannes Schindelin

2016-06-15 22:42 UTC #8 lore raw

Hi,

Quoted 7 lines · H. Peter Anvin

Quoted 6 lines · Johannes Schindelin
Something like this (totally untested):
 upload-pack.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

Quoted 13 lines

upload-pack.c static void create_pack_file(void) · line 124
Quoted 11 lines
upload-pack.c +1 -0
index 498bf50..bafd90f 100644
@@ -119,6 +119,8 @@ static void create_pack_file(void)
 		int i;
 		struct rev_info revs;
 +		close(0);
+
 		pack_pipe = fdopen(lp_pipe[1], "w");
  		if (create_full_pack)
Shouldn't that be close(lp_pipe[0]);?

Yes, of course!

Quoted 5 lines

upload-pack.c +1 -0

@@ -167,6 +169,10 @@ static void create_pack_file(void)
 		const char *argv[10];
 		int i = 0;
 +		close(0);
+		close(1);

Quoted 8 lines

upload-pack.c static void create_pack_file(void) · line 178
Quoted 6 lines
upload-pack.c +2 -0
+		close(2);
+
 		dup2(lp_pipe[0], 0);
 		dup2(pu_pipe[1], 1);
 		dup2(pe_pipe[1], 2);
Those close()'s are redundant with the dup2's...

I didn't know that, but that makes sense.

Ciao,
Dscho

H. Peter Anvin

author

2016-06-15 22:42 UTC #9 lore raw

In reply to Junio C Hamano

Quoted 5 lines · Junio C Hamano

Quoted 3 lines · H. Peter Anvin
... Going in with gdb, it appears the "timeout"
variable gets overwritten:

Quoted 7 lines

Quoted 4 lines
(gdb) p timeout
$1 = 608471321
... which should have been 600.
I do not see offhand what can cause this. The only new code since 1.4.4 series is "shallow clone" stuff,...

Well, we changed servers from an x86-64 box to an i386 box, so we're definitely running different binaries.

-hpa