This looks good, though if we're creating the temporary file in the same directory as the resulting file, which seems to be the case, could we not just os.rename() rather than calling a subprocess?

On Thu, Sep 16, 2021 at 7:25 AM Richard Purdie <richard.purdie@linuxfoundation.org> wrote:
There is a potential race over the mirror tarballs where a partial git repo
could be extracted causing fetcher failures if the tarball is being rewritten
whilst another build accesses it.

Create the mirror tarball atomically to avoid this.

[YOCTO #14441]

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 lib/bb/fetch2/git.py | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
index 488f4c7414..3643a491d7 100644
--- a/lib/bb/fetch2/git.py
+++ b/lib/bb/fetch2/git.py
@@ -68,6 +68,7 @@ import subprocess
 import tempfile
 import bb
 import bb.progress
+from contextlib import contextmanager
 from   bb.fetch2 import FetchMethod
 from   bb.fetch2 import runfetchcmd
 from   bb.fetch2 import logger
@@ -418,6 +419,20 @@ class Git(FetchMethod):
                 bb.utils.remove(tmpdir, recurse=True)

     def build_mirror_data(self, ud, d):
+
+        # Create as a temp file and move atomically into position to avoid races
+        @contextmanager
+        def create_atomic(filename, d):
+            fd, tfile = tempfile.mkstemp(dir=os.path.dirname(filename))
+            try:
+                yield tfile
+                umask = os.umask(0o666)
+                os.umask(umask)
+                os.chmod(tfile, (0o666 & ~umask))
+                runfetchcmd("mv %s %s" % (tfile, filename), d)
+            finally:
+                os.close(fd)
+
         if ud.shallow and ud.write_shallow_tarballs:
             if not os.path.exists(ud.fullshallow):
                 if os.path.islink(ud.fullshallow):
@@ -428,7 +443,8 @@ class Git(FetchMethod):
                     self.clone_shallow_local(ud, shallowclone, d)

                     logger.info("Creating tarball of git repository")
-                    runfetchcmd("tar -czf %s ." % ud.fullshallow, d, workdir=shallowclone)
+                    with create_atomic(ud.fullshallow, d) as tfile:
+                        runfetchcmd("tar -czf %s ." % tfile, d, workdir=shallowclone)
                     runfetchcmd("touch %s.done" % ud.fullshallow, d)
                 finally:
                     bb.utils.remove(tempdir, recurse=True)
@@ -437,7 +453,8 @@ class Git(FetchMethod):
                 os.unlink(ud.fullmirror)

             logger.info("Creating tarball of git repository")
-            runfetchcmd("tar -czf %s ." % ud.fullmirror, d, workdir=ud.clonedir)
+            with create_atomic(ud.fullmirror, d) as tfile:
+                runfetchcmd("tar -czf %s ." % tfile, d, workdir=ud.clonedir)
             runfetchcmd("touch %s.done" % ud.fullmirror, d)

     def clone_shallow_local(self, ud, dest, d):
--
2.32.0






--
Christopher Larson
kergoth at gmail dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Senior Software Engineer, Mentor Graphics