[bitbake-devel] [PATCH V2] fetch2/gitsm: fix config file concurrent update race

[Richard Purdie]

On Fri, 2019-01-25 at 15:41 +0100, liu.ming50 at gmail.com wrote:
> From: Ming Liu <liu.ming50 at gmail.com>
> 
> A following issue was observed with gitsm:
> > git -c core.fsyncobjectfiles=0 config submodule... failed with exit code 255, output:
> > error: could not lock config file config: File exists
> 
> Since all git submodules share a config file and Git creates a lock
> file (.git/config.lock) to synchronize access to it, but Git only tries
> exactly once and returns a error if it's already locked. This leads to
> a race condition if there are many 'git config submodule' run in
> different processes.
> 
> It could be fixed by introducing a bitbake file lock to protect config
> file from concurrent update from submodules.
> 
> Reported-by: Stefan Agner <stefan.agner at toradex.com>
> Signed-off-by: Ming Liu <liu.ming50 at gmail.com>
> ---
>  lib/bb/fetch2/gitsm.py | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/bb/fetch2/gitsm.py b/lib/bb/fetch2/gitsm.py
> index f45546b..9465acb 100644
> --- a/lib/bb/fetch2/gitsm.py
> +++ b/lib/bb/fetch2/gitsm.py
> @@ -183,11 +183,19 @@ class GitSM(Git):
>  
>              local_path = newfetch.localpath(url)
>  
> -            # Correct the submodule references to the local download version...
> -            runfetchcmd("%(basecmd)s config submodule.%(module)s.url %(url)s" % {'basecmd': ud.basecmd, 'module': module, 'url' : local_path}, d, workdir=ud.destdir)
> +            # Protects the config file from concurrent updates by submodules
> +            # e.g. 'git config submodule' run in different processes may lead
> +            # to a race condition.
> +            lf = bb.utils.lockfile(os.path.join(ud.destdir, "gitsm-config.lock"))
>  
> -            if ud.shallow:
> -                runfetchcmd("%(basecmd)s config submodule.%(module)s.shallow true" % {'basecmd': ud.basecmd, 'module': module}, d, workdir=ud.destdir)
> +            try:
> +                # Correct the submodule references to the local download version...
> +                runfetchcmd("%(basecmd)s config submodule.%(module)s.url %(url)s" % {'basecmd': ud.basecmd, 'module': module, 'url' : local_path}, d, workdir=ud.destdir)
> +
> +                if ud.shallow:
> +                    runfetchcmd("%(basecmd)s config submodule.%(module)s.shallow true" % {'basecmd': ud.basecmd, 'module': module}, d, workdir=ud.destdir)
> +            finally:
> +                bb.utils.unlockfile(lf)
>  
>              # Ensure the submodule repository is NOT set to bare, since we're checking it out...
>              try:

I still don't see why this is necessary after Mark's recent changes.
These files are in ud.destdir which in OE terms is WORKDIR. Only one
fetch happens against WORKDIR at a time? Therefore how is this racing?

Have you seen this problem against the current master code?

Cheers,

Richard