[PATCH] atomictempfile: avoid infinite recursion in destructor

Matt Mackall mpm at selenic.com
Mon Dec 13 10:08:14 CST 2010


On Mon, 2010-12-13 at 09:19 -0500, Greg Ward wrote:
> On Fri, Dec 10, 2010 at 4:57 PM, Matt Mackall <mpm at selenic.com> wrote:
> > On Mon, 2010-11-29 at 22:03 -0500, Greg Ward wrote:
> >> # HG changeset patch
> >> # User Greg Ward <greg-hg at gerg.ca>
> >> # Date 1291085861 18000
> >> # Branch stable
> >> # Node ID 76a458f91f5baad3dd75a26d445ea51402fc32d1
> >> # Parent  5fb924ee44d516e2f91bb7f5cfd5c81b0629faa4
> >> atomictempfile: avoid infinite recursion in destructor.
> >>
> >> This is a weird one: if Python sees that the caller is not passing the
> >> right args to the constructor, it raises TypeError rather than enter
> >> the constructor.  But it still ends up calling the destructor, which
> >> assumes that self._fp has been set in the constructor.  But it hasn't,
> >> because the constructor wasn't actually executed.  So we go into a
> >> nasty infinite recursion between __del__() and __getattr__(), when we
> >> should just crash with TypeError so the programmer can see what he did
> >> wrong.
> >
> > I think it's a bit too weird. Either we do this for all variables used
> > by all destructors, or skip it. As this only defends against explosively
> > incorrect code, I don't think we should worry about it.
> 
> There are so many levels of fail here that it's hard to explain, but let me try.
> 
> 1. extension author screws up call to atomictempfile
> 2. atomictempfile has an infinite recursion bug in that case
> 3. Python (?) makes the infinite recursion particularly hard to deal
> with -- sometime Ctrl-C works, but I have had to "kill -9" the
> offending process as well

You might be interested to learn about Ctrl-\.

> First: a minor error in client code (forgot a required parameter)
> should result in an ordinary TypeError crash, not an infinite
> recursion.  So atomictempfile magnifies my silly programmer error.  If
> I could kill the offending process with a single Ctrl-C, I would
> probably have left it at that.
> 
> But, for unknown reasons, the infinite recursion is hard to kill.  (I
> suspect there's something fishy about Python's signal handing around
> __del__(), but I have not investigated.)  So a simple programmer error
> is doubly magnified into a hard-to-kill runaway process.
> 
> I can't fix every future extension or script that accidentally leaves
> out a required arg to atomictempfile.  Nor can I go back in time and
> fix Python's signal handling in 2.4, 2.5, and 2.6.  But I *can* do
> something about atomictempfile, at least in Mercurial 1.8 (possibly
> 1.7.3).  Hence this patch.

Ok, so the minimal version of this bug looks like: 

class a(object):
    def __init__(self, b):
        self.a = b
    def __getattr__(self, name):
        return getattr(self.a, name)
    def __del__(self):
	print self.a

m = a()

The problem lies in how __getattr__ works and we could kill it with a
simple 'if name != "a":'. But this code is much better written as:

class b(file):
    def __init__(self, args):
        file.__init__(self, *args)
    def __del__(self):
        print self

m = b()

So if we're going to go to the trouble of fixing this ugly thing, I'd
rather fix it like this. We can do this to atomictempfile thusly and get
a cleaner result, but there are some issue with "posixfile" that need to
be sorted out here:

diff -r 97c5189b8607 mercurial/util.py
--- a/mercurial/util.py	Fri Dec 10 20:32:39 2010 -0600
+++ b/mercurial/util.py	Mon Dec 13 10:03:45 2010 -0600
@@ -800,7 +800,7 @@
         raise
     return temp
 
-class atomictempfile(object):
+class atomictempfile(file):
     """file-like object that atomically updates a file
 
     All writes will be redirected to a temporary copy of the original
@@ -808,28 +808,24 @@
     name, making the changes visible.
     """
     def __init__(self, name, mode='w+b', createmode=None):
+        temp = mktempcopy(name, emptyok=('w' in mode),
+                               createmode=createmode)
+        file.__init__(self, temp, mode)
         self.__name = name
-        self._fp = None
-        self.temp = mktempcopy(name, emptyok=('w' in mode),
-                               createmode=createmode)
-        self._fp = posixfile(self.temp, mode)
-
-    def __getattr__(self, name):
-        return getattr(self._fp, name)
+        self.__temp = temp
 
     def rename(self):
-        if not self._fp.closed:
-            self._fp.close()
-            rename(self.temp, localpath(self.__name))
+        if not self.closed:
+            file.close(self)
+            rename(self.__temp, localpath(self.__name))
 
     def close(self):
-        if not self._fp:
-            return
-        if not self._fp.closed:
+        if not self.closed:
             try:
                 os.unlink(self.temp)
-            except: pass
-            self._fp.close()
+            except:
+                pass
+            file.close(self)
 
     def __del__(self):
         self.close()


-- 
Mathematics is the supreme nostalgia of our time.




More information about the Mercurial-devel mailing list