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 Thu, 2023-03-30 at 10:10 +0100, Richard Purdie via
lists.openembedded.org wrote:
> 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

I should also mention, this test fails with oe-core's nodistro for
other reasons but does work ok with poky. I filed a bug for that:

https://bugzilla.yoctoproject.org/show_bug.cgi?id=15086

since it was confusing my debugging! You'd therefore need to use poky
for testing your change until we fix that.

Cheers,

Richard