* Fix a refleak when a preexec_fn was supplied (preexec_fn_args_tuple was not

  being defref'ed).
* Fixes another potential refleak of a reference to the gc
  module in the unlikely odd case where gc module isenabled or disable calls
  fail.
* Adds a unittest for the above case to verify behavior and lack of leaks.

2 files changed, 53 insertions, 3 deletions

diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py
index 74d9ce4..9f8512d 100644
--- a/Lib/test/test_subprocess.py
+++ b/Lib/test/test_subprocess.py
@@ -9,6 +9,10 @@ import tempfile
 import time
 import re
 import sysconfig
+try:
+    import gc
+except ImportError:
+    gc = None
 
 mswindows = (sys.platform == "win32")
 
@@ -650,6 +654,44 @@ class POSIXProcessTestCase(unittest.TestCase):
             self.fail("Exception raised by preexec_fn did not make it "
                       "to the parent process.")
 
+    @unittest.skipUnless(gc, "Requires a gc module.")
+    def test_preexec_gc_module_failure(self):
+        # This tests the code that disables garbage collection if the child
+        # process will execute any Python.
+        def raise_runtime_error():
+            raise RuntimeError("this shouldn't escape")
+        enabled = gc.isenabled()
+        orig_gc_disable = gc.disable
+        orig_gc_isenabled = gc.isenabled
+        try:
+            gc.disable()
+            self.assertFalse(gc.isenabled())
+            subprocess.call([sys.executable, '-c', ''],
+                            preexec_fn=lambda: None)
+            self.assertFalse(gc.isenabled(),
+                             "Popen enabled gc when it shouldn't.")
+
+            gc.enable()
+            self.assertTrue(gc.isenabled())
+            subprocess.call([sys.executable, '-c', ''],
+                            preexec_fn=lambda: None)
+            self.assertTrue(gc.isenabled(), "Popen left gc disabled.")
+
+            gc.disable = raise_runtime_error
+            self.assertRaises(RuntimeError, subprocess.Popen,
+                              [sys.executable, '-c', ''],
+                              preexec_fn=lambda: None)
+
+            del gc.isenabled  # force an AttributeError
+            self.assertRaises(AttributeError, subprocess.Popen,
+                              [sys.executable, '-c', ''],
+                              preexec_fn=lambda: None)
+        finally:
+            gc.disable = orig_gc_disable
+            gc.isenabled = orig_gc_isenabled
+            if not enabled:
+                gc.disable()
+
     def test_args_string(self):
         # args is a string
         fd, fname = mkstemp()
diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c
index f9b38b6..a6008ef 100644
--- a/Modules/_posixsubprocess.c
+++ b/Modules/_posixsubprocess.c
@@ -204,15 +204,21 @@ subprocess_fork_exec(PyObject* self, PyObject *args)
         if (gc_module == NULL)
             return NULL;
         result = PyObject_CallMethod(gc_module, "isenabled", NULL);
-        if (result == NULL)
+        if (result == NULL) {
+            Py_DECREF(gc_module);
             return NULL;
+        }
         need_to_reenable_gc = PyObject_IsTrue(result);
         Py_DECREF(result);
-        if (need_to_reenable_gc == -1)
+        if (need_to_reenable_gc == -1) {
+            Py_DECREF(gc_module);
             return NULL;
+        }
         result = PyObject_CallMethod(gc_module, "disable", NULL);
-        if (result == NULL)
+        if (result == NULL) {
+            Py_DECREF(gc_module);
             return NULL;
+        }
         Py_DECREF(result);
     }
 
@@ -307,6 +313,7 @@ subprocess_fork_exec(PyObject* self, PyObject *args)
         Py_XDECREF(gc_module);
         return NULL;
     }
+    Py_XDECREF(preexec_fn_args_tuple);
     Py_XDECREF(gc_module);
 
     if (pid == -1)
@@ -322,6 +329,7 @@ cleanup:
     _Py_FreeCharPArray(exec_array);
     Py_XDECREF(converted_args);
     Py_XDECREF(fast_args);
+    Py_XDECREF(preexec_fn_args_tuple);
 
     /* Reenable gc if it was disabled. */
     if (need_to_reenable_gc)