GUI: Canceling verification now also kills back-end thread.

Previously, pressing 'cancel' in the verification window would leave a dangling scyther backend process.
Now the process gets correctly killed.

The following changes enable this:
- External processes are no longer invoked through the shell (otherwise they are subprocesses of the shell and cannot be reliably killed cross-platform).
- The 'safeCommand' procedure now has a hook for passing opened Popen objects.
- The GUI stores and kills the Popen objects on cancel or window close.

To do: an alternative interface for this in 'safeCommand' could expose a 'killMe' method through a callback; this might be cleaner in the long term.

gui/Gui/Scytherthread.py

@@ -56,8 +56,21 @@ class ScytherThread(threading.Thread):
         self.options = options
         self.callback = callback
         self.mode = mode
+        self.popenList = []
         threading.Thread.__init__ ( self )
 
+    def storePopen(self,p):
+        self.popenList.append(p)
+
+    def cleanExit(self):
+        # Cleanup of spawned processes
+        for index,p in enumerate(self.popenList):
+            try:
+                p.kill()
+            except:
+                pass
+        self.popenList = []
+
     def run(self):
 
         (scyther, claims, summary) = self.claimResults()
@@ -100,7 +113,7 @@ class ScytherThread(threading.Thread):
 
         # verification start
         try:
-            claims = scyther.verify()
+            claims = scyther.verify(storePopen=self.storePopen)
         except Scyther.Error.ScytherError, el:
             claims = None
             pass
@@ -185,6 +198,9 @@ class VerificationWindow(wx.Dialog):
         self.SetSizer(sizer)
         sizer.Fit(self)
 
+        self.Center()
+        self.Show(True)
+
 #---------------------------------------------------------------------------
 
 class ErrorWindow(wx.Dialog):
@@ -398,7 +414,6 @@ class ResultWindow(wx.Frame):
 
 #---------------------------------------------------------------------------
 
-
 class ScytherRun(object):
 
     def __init__(self,mainwin,mode,spdl,errorcallback=None):
@@ -409,8 +424,23 @@ class ScytherRun(object):
         self.verified = False
         self.options = mainwin.settings.ScytherArguments(mode)
         self.errorcallback=errorcallback
+        self.SThread = None
+
         self.main()
 
+    def closer(self,ev):
+        # Triggered when the window is closed/verification cancelled
+        t = self.SThread
+        if t != None:
+            self.SThread = None
+            t.cleanExit()
+        try:
+            self.verifywin.Destroy()
+        except:
+            pass
+        self.verifywin = None
+        ev.Skip()
+
     def main(self):
         """
         Start process
@@ -422,8 +452,6 @@ class ScytherRun(object):
         # if the thread terminames, it should close the window normally, and we end up here as well.
         #val = self.verifywin.ShowModal()
         self.verifywin = VerificationWindow(self.mainwin,title)
-        self.verifywin.Center()
-        self.verifywin.Show(True)
 
         # Check sanity of Scyther thing here (as opposed to the thread)
         # which makes error reporting somewhat easier
@@ -435,10 +463,13 @@ class ScytherRun(object):
             Error.ShowAndExit(text)
         
         # start the thread
-        
         self.verifywin.SetCursor(wx.StockCursor(wx.CURSOR_WAIT))
-        t = ScytherThread(self.spdl, self.options, self.verificationDone, self.mode)
+        self.verifywin.Bind(wx.EVT_CLOSE, self.closer)
-        t.start()
+        self.verifywin.Bind(wx.EVT_WINDOW_DESTROY, self.closer)
+        self.verifywin.Bind(wx.EVT_BUTTON, self.closer, id=wx.ID_CANCEL)
+
+        self.SThread = ScytherThread(self.spdl, self.options, self.verificationDone, self.mode)
+        self.SThread.start()
 
         # after verification, we proceed to the callback below...
 
@@ -447,6 +478,9 @@ class ScytherRun(object):
         This is where we end up after a callback from the thread, stating that verification succeeded.
         """
 
+        if self.verifywin == None:
+            return
+
         self.scyther = scyther
         self.claims = claims
         self.summary = summary

gui/Scyther/Misc.py

@@ -26,6 +26,7 @@
 """ Import externals """
 import sys
 import os.path
+import shlex
 try:
     from subprocess import Popen,PIPE
 except:
@@ -89,35 +90,26 @@ def mypath(file):
     basedir = os.path.split(cmd_file)[0]
     return os.path.join(basedir,file)
 
-def getShell():
+def safeCommandOutput(cmd, storePopen=None):
-    """
-    Determine if we want a shell for Popen
-    """
-    if sys.platform.startswith("win"):
-        shell=False
-    else:
-        # Needed to handle the string input correctly (as opposed to a sequence where the first element is the executable)
-        # This is not needed on Windows, where it has a different effect altogether.
-        # See http://docs.python.org/library/subprocess.html?highlight=subprocess#module-subprocess
-        shell=True
-    return shell
-
-def safeCommandOutput(cmd):
     """ Execute a command and return (sts,sout,serr).
     Meant for short outputs, as output is stored in memory and
     not written to a file.
     """
-    p = Popen(cmd, shell=getShell(), stdout=PIPE, stderr=PIPE)
+    p = Popen(shlex.split(cmd), shell=False, stdout=PIPE, stderr=PIPE)
+    if storePopen != None:
+        storePopen(p)
     (sout,serr) = p.communicate()
 
     return (p.returncode,sout,serr)
 
-def safeCommand(cmd):
+def safeCommand(cmd, storePopen=None):
     """ Execute a command with some arguments. Safe cross-platform
     version, I hope. """
 
     try:
-        p = Popen(cmd, shell=getShell())
+        p = Popen(shlex.split(cmd), shell=False)
+        if storePopen != None:
+            storePopen(p)
         sts = p.wait()
     except KeyboardInterrupt, EnvironmentError:
         raise

gui/Scyther/Scyther.py

@@ -265,7 +265,7 @@ class Scyther(object):
         for arg in arglist:
             self.options += " %s" % (arg)
 
-    def doScytherCommand(self, spdl, args, checkKnown=False):
+    def doScytherCommand(self, spdl, args, checkKnown=False, storePopen=None):
         """
         Cached version of the 'real' below
 
@@ -290,7 +290,7 @@ class Scyther(object):
                 return False
             else:
                 # Need to compute
-                return self.doScytherCommandReal(spdl,args)
+                return self.doScytherCommandReal(spdl,args, storePopen=storePopen)
 
         # Apparently we are supporsed to be able to use the cache
         m = hashlib.sha256()
@@ -347,7 +347,7 @@ class Scyther(object):
             # We were only checking, abort
             return False
 
-        (out,err) = self.doScytherCommandReal(spdl,args)
+        (out,err) = self.doScytherCommandReal(spdl,args, storePopen=storePopen)
 
         try:
             # Try to store result in cache
@@ -366,13 +366,14 @@ class Scyther(object):
         return (out,err)
 
 
-    def doScytherCommandReal(self, spdl, args):
+    def doScytherCommandReal(self, spdl, args, storePopen=None):
         """ 
         Run Scyther backend on the input
         
         Arguments:
             spdl -- string describing the spdl text
             args -- arguments for the command-line
+            storePopen -- callback function to register Popen objects (used for process kill by other threads)
         Returns:
             (output,errors)
             output -- string which is the real output
@@ -415,7 +416,7 @@ class Scyther(object):
         ##print self.cmd
 
         # Start the process
-        safeCommand(self.cmd)
+        safeCommand(self.cmd, storePopen=storePopen)
 
         # reseek
         fhe = os.fdopen(fde)
@@ -437,7 +438,7 @@ class Scyther(object):
         """ Sanitize some of the input """
         self.options = EnsureString(self.options)
 
-    def verify(self,extraoptions=None,checkKnown=False):
+    def verify(self,extraoptions=None,checkKnown=False,storePopen=None):
         """ Should return a list of results """
         """ If checkKnown == True, we do not call Scyther, but just check the cache, and return True iff the result is in the cache """
 
@@ -455,10 +456,10 @@ class Scyther(object):
 
         # Are we only checking the cache?
         if checkKnown == True:
-            return self.doScytherCommand(self.spdl, args, checkKnown=checkKnown)
+            return self.doScytherCommand(self.spdl, args, checkKnown=checkKnown, storePopen=storePopen)
 
         # execute
-        (output,errors) = self.doScytherCommand(self.spdl, args)
+        (output,errors) = self.doScytherCommand(self.spdl, args, storePopen=storePopen)
         self.run = True
 
         # process errors
@@ -504,7 +505,7 @@ class Scyther(object):
         else:
             return self.output
 
-    def verifyOne(self,cl=None,checkKnown=False):
+    def verifyOne(self,cl=None,checkKnown=False,storePopen=None):
         """
         Verify just a single claim with an ID retrieved from the
         procedure below, 'scanClaims', or a full claim object
@@ -515,10 +516,10 @@ class Scyther(object):
             # We accept either a claim or a claim id
             if isinstance(cl,Claim.Claim):
                 cl = cl.id
-            return self.verify("--filter=%s" % cl, checkKnown=checkKnown)
+            return self.verify("--filter=%s" % cl, checkKnown=checkKnown,storePopen=storePopen)
         else:
             # If no claim, then its just normal verification
-            return self.verify(checkKnown=checkKnown)
+            return self.verify(checkKnown=checkKnown,storePopen=storePopen)
 
     def scanClaims(self):
         """