[PATCH] mkstemp: make sure we don't try to unlink files which were not created

Benoit Boissinot benoit.boissinot at ens-lyon.org
Sat Nov 7 06:39:13 CST 2009


[I randomly went over this patch, and thought it might be worth
responding]

On Wed, May 02, 2007 at 04:02:47PM +0300, Giorgos Keramidas wrote:
> # HG changeset patch
> # User Giorgos Keramidas <keramida at ceid.upatras.gr>
> # Date 1178110924 -10800
> # Node ID 52803b0fdf2e57f76b80ddc00c87809633b5b677
> # Parent  15289406f89c3866e5a1e1ce7d666a7555a23631
> mkstemp: make sure we don't try to unlink files which were not created
> 
> The tempfile.mkstemp() call may fail (i.e. because a file system is
> mounted as readonly after some file system errors).  In this case, we
> should not try to call os.unlink() or file.close() in finally: blocks.

Instead of moving the mkstemp() inside a try/finally, they should be put
outside when possible, so unlink()/close() won't be called.

> Inspired by:	trace posted by 'mathieu' on #mercurial

ie. the fix for the trace is to move the mkstemp outside.

(your proposed fix was actually added by Thomas in 3b6f18851d87, I'm
going to change it to the cleaner form in 0c84afa1d622)

Thanks (and sorry for not looking at the patch before).

Benoit

> diff --git a/mercurial/sshserver.py b/mercurial/sshserver.py
> --- a/mercurial/sshserver.py
> +++ b/mercurial/sshserver.py
> @@ -167,6 +167,7 @@ class sshserver(object):
>  
>          # write bundle data to temporary file because it can be big
>  
> +        fp, tempname = (None, None)
>          try:
>              fd, tempname = tempfile.mkstemp(prefix='hg-unbundle-')
>              fp = os.fdopen(fd, 'wb+')
> @@ -197,8 +198,10 @@ class sshserver(object):
>                      self.lock.release()
>                      self.lock = None
>          finally:
> -            fp.close()
> -            os.unlink(tempname)
> +            if fp is not None:
> +                fp.close()
> +            if tempname is not None:
> +                os.unlink(tempname)
>  
>      def do_stream_out(self):
>          streamclone.stream_out(self.repo, self.fout)
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -417,9 +417,10 @@ class ui(object):
>      def debug(self, *msg):
>          if self.debugflag: self.write(*msg)
>      def edit(self, text, user):
> -        (fd, name) = tempfile.mkstemp(prefix="hg-editor-", suffix=".txt",
> -                                      text=True)
> +        name = None
>          try:
> +            (fd, name) = tempfile.mkstemp(prefix="hg-editor-",
> +                                          suffix=".txt", text=True)
>              f = os.fdopen(fd, "w")
>              f.write(text)
>              f.close()
> @@ -437,7 +438,8 @@ class ui(object):
>              f.close()
>              t = re.sub("(?m)^HG:.*\n", "", t)
>          finally:
> -            os.unlink(name)
> +            if name is not None:
> +                os.unlink(name)
>  
>          return t
>  

-- 
:wq


More information about the Mercurial-devel mailing list