Opened 11 years ago

Closed 9 years ago

#2687 defect closed fixed (fixed)

Conch blows up when processes end due to signals

Reported by: Jonathan Lange Owned by:
Priority: normal Milestone:
Component: conch Keywords:
Cc: Branch: branches/session-signal-2687
branch-diff, diff-cov, branch-cov, buildbot
Author: therve

Description

RFC 4254, Section 6.10 says:

   The remote command may also terminate violently due to a signal.
   Such a condition can be indicated by the following message.  A zero
   'exit_status' usually means that the command terminated successfully.

      byte      SSH_MSG_CHANNEL_REQUEST
      uint32    recipient channel
      string    "exit-signal"
      boolean   FALSE
      string    signal name (without the "SIG" prefix)
      boolean   core dumped
      string    error message in ISO-10646 UTF-8 encoding
      string    language tag [RFC3066]

SSHSessionProcessProtocol says:

    def processEnded(self, reason = None):
        if reason and hasattr(reason.value, 'exitCode'):
            log.msg('exitCode: %s' % repr(reason.value.exitCode))
            self.session.conn.sendRequest(self.session, 'exit-status', struct.pack('!L', reason.value.exitCode))
        self.session.loseConnection()

Note that the ProcessTerminated exception will always have an exitCode attribute. If the process terminated due to a signal, then the exitCode attribute will be None, which cannot be packed into the struct. Conch should send an exit-signal message instead of an exit-status messge when exitCode is None.

Change History (15)

comment:1 Changed 11 years ago by Jonathan Lange

Basic patch to solve the explosion.

=== modified file 'twisted/conch/ssh/session.py'
--- twisted/conch/ssh/session.py        2006-03-16 08:44:37 +0000
+++ twisted/conch/ssh/session.py        2007-06-04 07:36:55 +0000
@@ -193,10 +193,14 @@
     def connectionLost(self, reason = None):
         self.session.loseConnection()
 
-    def processEnded(self, reason = None):
-        if reason and hasattr(reason.value, 'exitCode'): 
-            log.msg('exitCode: %s' % repr(reason.value.exitCode))
-            self.session.conn.sendRequest(self.session, 'exit-status', struct.pack('!L', reason.value.exitCode))
+    def processEnded(self, reason=None):
+        if reason:
+            exitCode = getattr(reason.value, 'exitCode', None)
+            if exitCode is not None:
+                log.msg('exitCode: %s' % repr(reason.value.exitCode))
+                self.session.conn.sendRequest(
+                    self.session, 'exit-status',
+                    struct.pack('!L', reason.value.exitCode))
         self.session.loseConnection()
 
     # transport stuff (we are also a transport!)

comment:2 Changed 10 years ago by Jonathan Lange

Keywords: review added
Priority: normalhighest

comment:3 Changed 10 years ago by therve

I don't see any test.

comment:4 Changed 10 years ago by Jonathan Lange

Right. I'd appreciate help writing the test.

comment:5 Changed 10 years ago by therve

Keywords: review removed
Owner: changed from z3p to therve
Priority: highestnormal

I guess I'll try.

comment:6 Changed 10 years ago by Jonathan Lange

You're a champion :)

comment:7 Changed 10 years ago by therve

This is done correctly (almost) in #2710. I think all the efforts should be into that direction.

comment:8 Changed 9 years ago by Jonathan Lange

This is a pretty trivial bug to fix, has been open for a year and is still biting us. I hereby express my renewed interest in fixing it personally.

comment:9 Changed 9 years ago by Jonathan Lange

launchpad_bug: 161743

comment:10 Changed 9 years ago by therve

Author: therve
Branch: branches/session-signal-2687

(In [27051]) Branching to 'session-signal-2687'

comment:11 Changed 9 years ago by therve

Keywords: review added
Owner: therve deleted

Let's revive that, from #2710 grave.

comment:12 Changed 9 years ago by Jonathan Lange

I did a review of the branch face-to-face with Thomas, and he's corrected all the comments I've had.

comment:13 Changed 9 years ago by Jonathan Lange

Keywords: review removed

comment:14 Changed 9 years ago by therve

Resolution: fixed
Status: newclosed

(In [27060]) Merge session-signal-2687

Author: therve Reviewer: jml Fixes #2687

Manage process termination in Conch due to signals, sending exit-signal message instead of exit-status.

comment:15 Changed 7 years ago by <automation>

Note: See TracTickets for help on using tickets.