From: Rob Browning Date: Fri, 24 Jun 2022 16:41:36 +0000 (-0500) Subject: atomically_replaced_file: respect umask/sgid/etc. via tmpdir X-Git-Url: https://arthur.barton.de/cgi-bin/gitweb.cgi?p=bup.git;a=commitdiff_plain;h=80f7aa7c3b3935f8f344e1870400ae9512b96a3a atomically_replaced_file: respect umask/sgid/etc. via tmpdir Don't create the tempfile via mkstemp because it always creates files with restricted permissions, which is not what we want for a new packfile (for example). The replacement files should respect the environment umask, directory sgid bits, etc. Instead, create a normally open()ed file in a mkdtemp directory in the same directory as the target path. Don't use TemporaryDirectory because it's a @contextmanager (see below). Add a missing '+' to the midx open mode. Without it mmap_readwrite's mmap will fail with EACCES. This wasn't an issue with the previous implementation because mkstemp doesn't accept a full mode string. Also drop @contextmanager. Because it involves a generator, @contextmanager creates managers that are incompatible with ExitStack.pop_all() because they close during that call -- exactly what pop_all() is intended to avoid. cf. https://github.com/python/cpython/issues/88458 Signed-off-by: Rob Browning Tested-by: Rob Browning --- diff --git a/lib/bup/cmd/midx.py b/lib/bup/cmd/midx.py index 81ffae7..1ff77f2 100644 --- a/lib/bup/cmd/midx.py +++ b/lib/bup/cmd/midx.py @@ -125,7 +125,7 @@ def _do_midx(outdir, outfilename, infilenames, prefixstr, debug1('midx: table size: %d (%d bits)\n' % (entries*4, bits)) unlink(outfilename) - with atomically_replaced_file(outfilename, 'wb') as f: + with atomically_replaced_file(outfilename, 'w+b') as f: f.write(b'MIDX') f.write(struct.pack('!II', midx.MIDX_VERSION, bits)) assert(f.tell() == 12) diff --git a/lib/bup/helpers.py b/lib/bup/helpers.py index a14cb1e..3636c13 100644 --- a/lib/bup/helpers.py +++ b/lib/bup/helpers.py @@ -7,8 +7,10 @@ from ctypes import sizeof, c_void_p from math import floor from os import environ from subprocess import PIPE, Popen +from tempfile import mkdtemp +from shutil import rmtree import sys, os, subprocess, errno, select, mmap, stat, re, struct -import hashlib, heapq, math, operator, time, tempfile +import hashlib, heapq, math, operator, time from bup import _helpers from bup import io @@ -41,6 +43,14 @@ class finalized: def __exit__(self, exc_type, exc_value, traceback): self.finalize(self.enter_result) +def temp_dir(*args, **kwargs): + # This is preferable to tempfile.TemporaryDirectory because the + # latter uses @contextmanager, and so will always eventually be + # deleted if it's handed to an ExitStack, whenever the stack is + # gc'ed, even if you pop_all() (the new stack will also trigger + # the deletion) because + # https://github.com/python/cpython/issues/88458 + return finalized(mkdtemp(*args, **kwargs), lambda x: rmtree(x)) sc_page_size = os.sysconf('SC_PAGE_SIZE') assert(sc_page_size > 0) @@ -716,40 +726,52 @@ def chunkyreader(f, count = None): yield b -@contextmanager -def atomically_replaced_file(name, mode='w', buffering=-1): - """Yield a file that will be atomically renamed name when leaving the block. - - This contextmanager yields an open file object that is backed by a - temporary file which will be renamed (atomically) to the target - name if everything succeeds. - - The mode and buffering arguments are handled exactly as with open, - and the yielded file will have very restrictive permissions, as - per mkstemp. - - E.g.:: - - with atomically_replaced_file('foo.txt', 'w') as f: - f.write('hello jack.') - - """ - - (ffd, tempname) = tempfile.mkstemp(dir=os.path.dirname(name), - text=('b' not in mode)) - try: - try: - f = os.fdopen(ffd, mode, buffering) - except: - os.close(ffd) - raise - try: - yield f - finally: - f.close() - os.rename(tempname, name) - finally: - unlink(tempname) # nonexistant file is ignored +class atomically_replaced_file: + def __init__(self, path, mode='w', buffering=-1): + """Return a context manager supporting the atomic replacement of a file. + + The context manager yields an open file object that has been + created in a mkdtemp-style temporary directory in the same + directory as the path. The temporary file will be renamed to + the target path (atomically if the platform allows it) if + there are no exceptions, and the temporary directory will + always be removed. Calling cancel() will prevent the + replacement. + + The file object will have a name attribute containing the + file's path, and the mode and buffering arguments will be + handled exactly as with open(). The resulting permissions + will also match those produced by open(). + + E.g.:: + + with atomically_replaced_file('foo.txt', 'w') as f: + f.write('hello jack.') + + """ + assert 'w' in mode + self.path = path + self.mode = mode + self.buffering = buffering + self.canceled = False + self.tmp_path = None + self.cleanup = ExitStack() + def __enter__(self): + with self.cleanup: + parent, name = os.path.split(self.path) + tmpdir = self.cleanup.enter_context(temp_dir(dir=parent, + prefix=name + b'-')) + self.tmp_path = tmpdir + b'/pending' + f = open(self.tmp_path, mode=self.mode, buffering=self.buffering) + f = self.cleanup.enter_context(f) + self.cleanup = self.cleanup.pop_all() + return f + def __exit__(self, exc_type, exc_value, traceback): + with self.cleanup: + if not (self.canceled or exc_type): + os.rename(self.tmp_path, self.path) + def cancel(self): + self.canceled = True def slashappend(s):