[PATCH] fetch2/git: Avoid races over mirror tarball creation

[PATCH] fetch2/git: Avoid races over mirror tarball creation

[Richard Purdie]

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

Re: [bitbake-devel] [PATCH] fetch2/git: Avoid races over mirror tarball creation

[Christopher Larson]

[-- Attachment #1: Type: text/plain, Size: 3366 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 4484 bytes --]

Re: [bitbake-devel] [PATCH] fetch2/git: Avoid races over mirror tarball creation

[Richard Purdie]

On Fri, 2021-09-17 at 09:50 -0700, Christopher Larson wrote:
> 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?

I meant to check that, I had some recollection the rename might not work if the
file already exists in the os.rename() case. I got distracted by other things
and didn't get back to it but it looks like that should be fine glancing at the
manual.

Cheers,

Richard