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

["Christopher Larson" <kergoth@gmail.com>]

[-- 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 --]