Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#4350 defect closed fixed (fixed)

Logic error in SSHSessionProcessProtocol.inConnectionLost()

Reported by: bshi Owned by:
Priority: normal Milestone:
Component: conch Keywords:
Cc: Branch: branches/ssh-session-close-4350
(diff, github, buildbot, log)
Author: therve Launchpad Bug:

Description

The program gitconnbug.py (attached) wraps git-shell and and can be executed to start the sample server. Run a command like

# The /abspath/to/git/repo should be readable by the server user
$ git clone ssh://user@localhost:2222/abspath/to/git/repo
(password "user")

To test the server. I have encountered an issue that would cause a git client fail in an apparently non-deterministic manner. On my workstation, I am able to reproduce the error at least once every 5 tries. The behavior I see is attached at the end of this email. As far as I can tell, the client fails due to a premature inConnectionLost() call in the SSHSessionProcessProtocol that sends an EOF.

As a workaround, when TraceProcessProtocol.inConnectionLost() is overriden to do nothing, this error goes away.

Successful case (server side logging, timestamp cut out):

[SSHChannel session (0) on SSHService ssh-connection
on SSHServerTransport,1,127.0.0.1] executing command
"git-upload-pack '/Users/bshi/sandbox/poop'"
[-] TPP.outReceived(...) 199 bytes
[-] TPP.outReceived(...) 146 bytes
[-] TPP.outReceived(...) 8 bytes
[-] TPP.outReceived(...) 33 bytes
[-] TPP.outReceived(...) 41 bytes
[-] TPP.outReceived(...) 77 bytes
[-] TPP.outReceived(...) 1228 bytes
[-] TPP.outReceived(...) 8192 bytes
[-] TPP.outReceived(...) 4567 bytes
[-] TPP.inConnectionLost()
[-] sending eof
[-] exitCode: 0

Successful case (client-side shell session):

$ GIT_TRACE=1 git clone ssh://user@localhost:2222/Users/bshi/sandbox/poop

trace: built-in: git 'clone' 'ssh://user@localhost:2222/Users/bshi/sandbox/poop'
Initialized empty Git repository in /tmp/poop/.git/
trace: run_command: 'ssh' '-p' '2222' 'user@localhost' 'git-upload-pack
       '\''/Users/bshi/sandbox/poop'\'''
user@localhost's password:
trace: run_command: 'index-pack' '--stdin' '-v' '--fix-thin'
             '--keep=fetch-pack 763 on Bo-Shis-MacBook-Pro.local'
trace: exec: 'git' 'index-pack' '--stdin' '-v' '--fix-thin'
             '--keep=fetch-pack 763 on Bo-Shis-MacBook-Pro.local'
trace: exec: 'git-index-pack' '--stdin' '-v' '--fix-thin'
             '--keep=fetch-pack 763 on Bo-Shis-MacBook-Pro.local'
trace: run_command: 'git-index-pack' '--stdin' '-v' '--fix-thin'
             '--keep=fetch-pack 763 on Bo-Shis-MacBook-Pro.local'
remote: Counting objects: 50, done.
remote: Compressing objects: 100% (35/35), done.
remote: Total 50 (delta 16), reused 43 (delta 14)
Receiving objects: 100% (50/50), 12.36 KiB, done.
Resolving deltas: 100% (16/16), done.

Failure case (server side):

   [SSHChannel session (0) on SSHService ssh-connection
   on SSHServerTransport,2,127.0.0.1] executing command
   "git-upload-pack '/Users/bshi/sandbox/poop'"
   [-] TPP.outReceived(...) 345 bytes
   [-] TPP.outReceived(...) 8 bytes
   [-] TPP.outReceived(...) 33 bytes
   [-] TPP.outReceived(...) 8192 bytes
   [-] TPP.inConnectionLost()
   [-] sending eof
   [-] TPP.outReceived(...) 5903 bytes
   [-] exitCode: 0
   [-] sending request exit-status
   [-] sending close 0

Failure case ("trace:" messages similar, cut out for readability):

   $ GIT_TRACE=1 git clone ssh://user@localhost:2222/Users/bshi/sandbox/poop
   user@localhost's password:
   ...
   ...
   remote: Counting objects: 50, done.
   fatal: The remote end hung up unexpectedly
   fatal: early EOF
   fatal: index-pack failed

Attachments (2)

gitconnbug.py (5.3 KB) - added by bshi 5 years ago.
ssh server reproducing issue (port 2222)
sshpp.patch (2.4 KB) - added by bshi 5 years ago.

Download all attachments as: .zip

Change History (8)

Changed 5 years ago by bshi

ssh server reproducing issue (port 2222)

Changed 5 years ago by bshi

comment:1 Changed 5 years ago by bshi

  • Keywords review added
  • Owner z3p deleted

Proposed patch based on James' and Michael's comments at

http://twistedmatrix.com/pipermail/twisted-python/2010-March/021776.html

The patch makes the assumption that outConnectionLost() and errConnectionLost() can only be called once for a given processprotocol instance. If this is not true, I need to reword things a bit.

comment:2 Changed 5 years ago by bshi

  • Summary changed from SSHSessionProcessProtocol.inConnectionLost() behavior breaks Git over Conch to Logic error in SSHSessionProcessProtocol.inConnectionLost()

comment:3 Changed 5 years ago by therve

  • Author set to therve
  • Branch set to branches/ssh-session-close-4350

(In [28797]) Branching to 'ssh-session-close-4350'

comment:4 Changed 5 years ago by therve

  • Resolution set to fixed
  • Status changed from new to closed

(In [28800]) Merge ssh-session-close-4350

Author: bshi
Reviewer: therve
Fixes: #4350

Fix a behavior in SSHSessionProcessProtocol, which used to close the session
when stdin is closed, and now closes when stdout and stderr are closed.

comment:5 Changed 5 years ago by therve

  • Keywords review removed

Thanks a lot for the ticket and the patch!

comment:6 Changed 4 years ago by <automation>

Note: See TracTickets for help on using tickets.