Re: [OE-core] [PATCH] lib/oe/gpg_sign.py: Avoid race when creating .sig files in detach_sign

[Richard Purdie <richard.purdie@linuxfoundation.org>]

On Wed, 2023-03-29 at 23:33 +0100, Richard Purdie via
lists.openembedded.org wrote:
> On Thu, 2023-03-23 at 11:08 +0100, Tobias Hagelborn wrote:
> > Move the signature file into place only after it is successfully signed.
> > This to avoid race and corrupted .sig files in cases multiple onging
> > builds write to a shared sstate-cache dir.
> > 
> > Signed-off-by: Tobias Hagelborn <tobiasha@axis.com>
> > ---
> >  meta/lib/oe/gpg_sign.py | 25 +++++++++++++++----------
> >  1 file changed, 15 insertions(+), 10 deletions(-)
> > 
> > diff --git a/meta/lib/oe/gpg_sign.py b/meta/lib/oe/gpg_sign.py
> > index 613dab8561..868846cdc5 100644
> > --- a/meta/lib/oe/gpg_sign.py
> > +++ b/meta/lib/oe/gpg_sign.py
> > @@ -5,11 +5,12 @@
> >  #
> >  
> >  """Helper module for GPG signing"""
> > -import os
> >  
> >  import bb
> > -import subprocess
> > +import os
> >  import shlex
> > +import subprocess
> > +import tempfile
> >  
> >  class LocalSigner(object):
> >      """Class for handling local (on the build host) signing"""
> > @@ -73,8 +74,6 @@ class LocalSigner(object):
> >              cmd += ['--homedir', self.gpg_path]
> >          if armor:
> >              cmd += ['--armor']
> > -        if output_suffix:
> > -            cmd += ['-o', input_file + "." + output_suffix]
> >          if use_sha256:
> >              cmd += ['--digest-algo', "SHA256"]
> >  
> > @@ -83,19 +82,25 @@ class LocalSigner(object):
> >          if self.gpg_version > (2,1,):
> >              cmd += ['--pinentry-mode', 'loopback']
> >  
> > -        cmd += [input_file]
> > -
> >          try:
> >              if passphrase_file:
> >                  with open(passphrase_file) as fobj:
> >                      passphrase = fobj.readline();
> >  
> > -            job = subprocess.Popen(cmd, stdin=subprocess.PIPE, stderr=subprocess.PIPE)
> > -            (_, stderr) = job.communicate(passphrase.encode("utf-8"))
> > +            output_file = input_file + "." + (output_suffix or 'sig')
> 
> This doesn't match the behaviour of output_suffix as used above where
> it defaults to None. This forces it to a default of "sig" instead?
> 
> If that intentional that should be in the commit message.
> 
> 
> > +            with tempfile.TemporaryDirectory(dir=os.path.dirname(output_file)) as tmp_dir:
> > +                tmp_file = os.path.join(tmp_dir, os.path.basename(output_file))
> > +                cmd += ['-o', tmp_file]
> > +
> > +                cmd += [input_file]
> > +
> > +                job = subprocess.Popen(cmd, stdin=subprocess.PIPE, stderr=subprocess.PIPE)
> > +                (_, stderr) = job.communicate(passphrase.encode("utf-8"))
> >  
> > -            if job.returncode:
> > -                bb.fatal("GPG exited with code %d: %s" % (job.returncode, stderr.decode("utf-8")))
> > +                if job.returncode:
> > +                    bb.fatal("GPG exited with code %d: %s" % (job.returncode, stderr.decode("utf-8")))
> >  
> > +                os.rename(tmp_file, output_file)
> >          except IOError as e:
> >              bb.error("IO error (%s): %s" % (e.errno, e.strerror))
> >              raise Exception("Failed to sign '%s'" % input_file)

I've been struggling to confirm it but have now done so. This does
cause an oe-selftest failure as here:

https://autobuilder.yoctoproject.org/typhoon/#/builders/80/builds/4950/steps/14/logs/stdio


i.e. from

oe-selftest -r runtime_test.TestImage.test_testimage_dnf

Cheers,

Richard