[PATCH] fetch2/gitsm: fix config file concurrent update race

[PATCH] fetch2/gitsm: fix config file concurrent update race

[liu.ming50]

From: Ming Liu <liu.ming50@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@toradex.com>
Signed-off-by: Ming Liu <liu.ming50@gmail.com>
---
 lib/bb/fetch2/gitsm.py | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/lib/bb/fetch2/gitsm.py b/lib/bb/fetch2/gitsm.py
index f45546b..bb49d5c 100644
--- a/lib/bb/fetch2/gitsm.py
+++ b/lib/bb/fetch2/gitsm.py
@@ -39,6 +39,13 @@ from   bb.fetch2 import Fetch
 from   bb.fetch2 import BBFetchException
 
 class GitSM(Git):
+    def urldata_init(self, ud, d):
+        super().urldata_init(ud, d)
+        # Protects the config file from concurrent updates by submodules
+        # e.g. 'git config submodule' run in different processes may lead
+        # to a race condition.
+        ud.gitsmlock = os.path.join(ud.clonedir, "gitsm-config.lock")
+
     def supports(self, ud, d):
         """
         Check to see if a given url can be fetched with git.
@@ -183,11 +190,15 @@ 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)
+            lf = bb.utils.lockfile(ud.gitsmlock)
+            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)
+                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:
-- 
2.7.4

Re: [PATCH] fetch2/gitsm: fix config file concurrent update race

[Richard Purdie]

On Fri, 2019-01-25 at 11:57 +0100, liu.ming50@gmail.com wrote:
> From: Ming Liu <liu.ming50@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@toradex.com>
> Signed-off-by: Ming Liu <liu.ming50@gmail.com>
> ---
>  lib/bb/fetch2/gitsm.py | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)

This issue should already have been fixed with recent changes to the
gitsm fetcher. Could you confirm which version of the code you were
seeing this problem with?

Cheers,

Richard

Re: [PATCH] fetch2/gitsm: fix config file concurrent update race

[Ming Liu]

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

Hi, Stefan:

Could you help confirm this?

Hi, Richard:

May I know how has the issue been fixed?

I updated the tip to:
```
commit 8c8ecec2a722bc2885e2648d41ac8df07bdf660d
    gitsmy.py: Fix unpack of submodules of submodules

    If the submodule is in a subdirectory, it needs to have that structure
    preserved.  This means the unpack path needs to be in the 'dirname' of
the
    final path -- since the unpack directory name is specified in the URI.

    Additional specific test cases were added to ensure this is working
properly
    based on two recent error reports.

    Signed-off-by: Mark Hatle <mark.hatle@windriver.com>
    Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
```
on top of that commit, I could see there are "git config submodule" being
called concurrently in different processes, which does have a race
condition, and will lead to the error observed by Stefan, although it's not
easy to be reproduced.

//Ming Liu



Richard Purdie <richard.purdie@linuxfoundation.org> 於 2019年1月25日 週五
下午12:18寫道：

> On Fri, 2019-01-25 at 11:57 +0100, liu.ming50@gmail.com wrote:
> > From: Ming Liu <liu.ming50@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@toradex.com>
> > Signed-off-by: Ming Liu <liu.ming50@gmail.com>
> > ---
> >  lib/bb/fetch2/gitsm.py | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
>
> This issue should already have been fixed with recent changes to the
> gitsm fetcher. Could you confirm which version of the code you were
> seeing this problem with?
>
> Cheers,
>
> Richard
>
>

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

Re: [PATCH] fetch2/gitsm: fix config file concurrent update race

[Richard Purdie]

On Fri, 2019-01-25 at 12:35 +0100, Ming Liu wrote:
> Hi, Stefan:
> 
> Could you help confirm this? 
> 
> Hi, Richard:
> 
> May I know how has the issue been fixed?
>
> I updated the tip to:
> ```
> commit 8c8ecec2a722bc2885e2648d41ac8df07bdf660d
>     gitsmy.py: Fix unpack of submodules of submodules
>     
>     If the submodule is in a subdirectory, it needs to have that
> structure
>     preserved.  This means the unpack path needs to be in the
> 'dirname' of the
>     final path -- since the unpack directory name is specified in the
> URI.
>     
>     Additional specific test cases were added to ensure this is
> working properly
>     based on two recent error reports.
>     
>     Signed-off-by: Mark Hatle <mark.hatle@windriver.com>
>     Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org
> >
> ```
> on top of that commit, I could see there are "git config submodule"
> being called concurrently in different processes, which does have a
> race condition, and will lead to the error observed by Stefan,
> although it's not easy to be reproduced.

There are locks at a higher level in the fetchers, in
lib/bb/fetch2/__init__.py the download() method has:

if ud.lockfile:
    lf = bb.utils.lockfile(ud.lockfile)

and the unpack() method has similar locks.

Mark and I believe there were some code paths where the locks were
being bypassed in the gitsm fetcher but Mark reworked the code so that
we were both happy that there should always be a lock held.

We need to fix any races properly ensuring they're covered by the main
fetcher locks, adding new ones would only fix a symptom of the real
problem.

Cheers,

Richard

Re: [PATCH] fetch2/gitsm: fix config file concurrent update race

[Ming Liu]

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

Hi, Richard:

Involve in Mark.

Please correct me if I am wrong, but according to the code, seems these
upper level locks could not protect this race condition, because it's
inside the GitSM unpack function, the call chain is:

Inside GitSM:
```
def unpack
  -> for module in submodules:
       -> unpack_submodules
            -> runfetchcmd
                 -> bb.process.run
                      -> git config submodule
```

since 'git config submodule' run concurrently and try to modify a same git
config file, hence the race might happen, but it's hard to be reproduced
since 'git config submodule' finishes quite quickly.

//Ming Liu

Richard Purdie <richard.purdie@linuxfoundation.org> 於 2019年1月25日 週五
下午1:24寫道：

> On Fri, 2019-01-25 at 12:35 +0100, Ming Liu wrote:
> > Hi, Stefan:
> >
> > Could you help confirm this?
> >
> > Hi, Richard:
> >
> > May I know how has the issue been fixed?
> >
> > I updated the tip to:
> > ```
> > commit 8c8ecec2a722bc2885e2648d41ac8df07bdf660d
> >     gitsmy.py: Fix unpack of submodules of submodules
> >
> >     If the submodule is in a subdirectory, it needs to have that
> > structure
> >     preserved.  This means the unpack path needs to be in the
> > 'dirname' of the
> >     final path -- since the unpack directory name is specified in the
> > URI.
> >
> >     Additional specific test cases were added to ensure this is
> > working properly
> >     based on two recent error reports.
> >
> >     Signed-off-by: Mark Hatle <mark.hatle@windriver.com>
> >     Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org
> > >
> > ```
> > on top of that commit, I could see there are "git config submodule"
> > being called concurrently in different processes, which does have a
> > race condition, and will lead to the error observed by Stefan,
> > although it's not easy to be reproduced.
>
> There are locks at a higher level in the fetchers, in
> lib/bb/fetch2/__init__.py the download() method has:
>
> if ud.lockfile:
>     lf = bb.utils.lockfile(ud.lockfile)
>
> and the unpack() method has similar locks.
>
> Mark and I believe there were some code paths where the locks were
> being bypassed in the gitsm fetcher but Mark reworked the code so that
> we were both happy that there should always be a lock held.
>
> We need to fix any races properly ensuring they're covered by the main
> fetcher locks, adding new ones would only fix a symptom of the real
> problem.
>
> Cheers,
>
> Richard
>
>
>
>

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

Re: [PATCH] fetch2/gitsm: fix config file concurrent update race

[Richard Purdie]

On Fri, 2019-01-25 at 14:44 +0100, Ming Liu wrote:
> Hi, Richard:
> 
> Involve in Mark.
> 
> Please correct me if I am wrong, but according to the code, seems
> these upper level locks could not protect this race condition,
> because it's inside the GitSM unpack function, the call chain is:
> 
> Inside GitSM:
> ```
> def unpack
>   -> for module in submodules:
>        -> unpack_submodules 
>             -> runfetchcmd
>                  -> bb.process.run
>                       -> git config submodule
> ```
> 
> since 'git config submodule' run concurrently and try to modify a
> same git config file, hence the race might happen, but it's hard to
> be reproduced since 'git config submodule' finishes quite quickly.

for that git config call, workdir=ud.destdir so its not in any shared
location like DL_DIR but in a recipe's workdir?

Cheers,

Richard

Re: [PATCH] fetch2/gitsm: fix config file concurrent update race

[Ming Liu]

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

Oops, you are absolutely correct, will send a V2 fixing that.

We can continue this discussion on V2.

//Ming Liu

Richard Purdie <richard.purdie@linuxfoundation.org>于2019年1月25日 周五15:10写道：

> On Fri, 2019-01-25 at 14:44 +0100, Ming Liu wrote:
> > Hi, Richard:
> >
> > Involve in Mark.
> >
> > Please correct me if I am wrong, but according to the code, seems
> > these upper level locks could not protect this race condition,
> > because it's inside the GitSM unpack function, the call chain is:
> >
> > Inside GitSM:
> > ```
> > def unpack
> >   -> for module in submodules:
> >        -> unpack_submodules
> >             -> runfetchcmd
> >                  -> bb.process.run
> >                       -> git config submodule
> > ```
> >
> > since 'git config submodule' run concurrently and try to modify a
> > same git config file, hence the race might happen, but it's hard to
> > be reproduced since 'git config submodule' finishes quite quickly.
>
> for that git config call, workdir=ud.destdir so its not in any shared
> location like DL_DIR but in a recipe's workdir?
>
> Cheers,
>
> Richard
>
>

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

Re: [PATCH] fetch2/gitsm: fix config file concurrent update race

[Mark Hatle]

On 1/25/19 5:35 AM, Ming Liu wrote:
> Hi, Stefan:
> 
> Could you help confirm this? 
> 
> Hi, Richard:
> 
> May I know how has the issue been fixed?

http://git.openembedded.org/bitbake/commit/?id=346338667edca1f58ace769ad417548da2b8d981

http://git.openembedded.org/bitbake/commit/?id=610dbee5634677f5055e2b36a3043cd197fb8c51

Those two commits will eliminate the place in the code where the error was
coming from.

There are only three remaining calls to git config, and they ONLY happen during
unpack.  unpack is safe since there can not be two unpack tasks running against
the same workdir at the same time without other errors occurring as well.  (That
is invalid.)

--Mark

> I updated the tip to:
> ```
> commit 8c8ecec2a722bc2885e2648d41ac8df07bdf660d
>     gitsmy.py: Fix unpack of submodules of submodules
>     
>     If the submodule is in a subdirectory, it needs to have that structure
>     preserved.  This means the unpack path needs to be in the 'dirname' of the
>     final path -- since the unpack directory name is specified in the URI.
>     
>     Additional specific test cases were added to ensure this is working properly
>     based on two recent error reports.
>     
>     Signed-off-by: Mark Hatle <mark.hatle@windriver.com
> <mailto:mark.hatle@windriver.com>>
>     Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org
> <mailto:richard.purdie@linuxfoundation.org>>
> ```
> on top of that commit, I could see there are "git config submodule" being called
> concurrently in different processes, which does have a race condition, and will
> lead to the error observed by Stefan, although it's not easy to be reproduced.
> 
> //Ming Liu
> 
> 
> 
> Richard Purdie <richard.purdie@linuxfoundation.org
> <mailto:richard.purdie@linuxfoundation.org>> 於 2019年1月25日 週五 下午12:18寫道：
> 
>     On Fri, 2019-01-25 at 11:57 +0100, liu.ming50@gmail.com
>     <mailto:liu.ming50@gmail.com> wrote:
>     > From: Ming Liu <liu.ming50@gmail.com <mailto:liu.ming50@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@toradex.com
>     <mailto:stefan.agner@toradex.com>>
>     > Signed-off-by: Ming Liu <liu.ming50@gmail.com <mailto:liu.ming50@gmail.com>>
>     > ---
>     >  lib/bb/fetch2/gitsm.py | 19 +++++++++++++++----
>     >  1 file changed, 15 insertions(+), 4 deletions(-)
> 
>     This issue should already have been fixed with recent changes to the
>     gitsm fetcher. Could you confirm which version of the code you were
>     seeing this problem with?
> 
>     Cheers,
> 
>     Richard
> 
> 

Re: [PATCH] fetch2/gitsm: fix config file concurrent update race

[Mark Hatle]

On 1/25/19 7:44 AM, Ming Liu wrote:
> Hi, Richard:
> 
> Involve in Mark.
> 
> Please correct me if I am wrong, but according to the code, seems these upper
> level locks could not protect this race condition, because it's inside the GitSM
> unpack function, the call chain is:

original code, where the error occurred happened during -fetch-.  Somehow, two
fetches happened at the same time and tried to modify a shared download
location.  It was determined, the most likely cause of this was from the
previous 'need_update' function, which may or may not have been locking things
properly.  (That was never determined).

The patch below removes that function as no longer necessary.

http://git.openembedded.org/bitbake/commit/?id=346338667edca1f58ace769ad417548da2b8d981

Further... all other config calls that happened during fetch were removed in:

http://git.openembedded.org/bitbake/commit/?id=610dbee5634677f5055e2b36a3043cd197fb8c51

The config call with a target path of ud.clonedir, specifically:

-            # 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_paths[module]}, d,
workdir=ud.clonedir)

were removed.. ensuring that no git config calls during fetch, targeting the
clone directory [download dir] can occur.  (All other actions happen based on
the regular git fetcher invocations.)

Thus there are no longer calls to git config during the fetch operation to a
shared location.  This leaves unpack as the only place that git config is invoked...

> Inside GitSM:
> ```
> def unpack
>   -> for module in submodules:
>        -> unpack_submodules 
>             -> runfetchcmd
>                  -> bb.process.run
>                       -> git config submodule
> ```
> 
> since 'git config submodule' run concurrently and try to modify a same git
> config file, hence the race might happen, but it's hard to be reproduced since
> 'git config submodule' finishes quite quickly.

git config runs only under the unpack operations, with a working directory
located within the WORKDIR of the recipe.  The only way to end up with two
processes calling git config, in the same workdir, at the same time is if two
unpack tasks are running against the same WORKDIR.  This itself is an error, and
must never happened for a functioning system.

(The only time this -could- happen is if you have a shared workdir, but the
locking on the shared workdir itself will prevent this operation.  If you recipe
is lacking the appropriate locking there it is a bug in the recipe implementation.)

--Mark

> //Ming Liu
> 
> Richard Purdie <richard.purdie@linuxfoundation.org
> <mailto:richard.purdie@linuxfoundation.org>> 於 2019年1月25日 週五 下午1:24寫道：
> 
>     On Fri, 2019-01-25 at 12:35 +0100, Ming Liu wrote:
>     > Hi, Stefan:
>     >
>     > Could you help confirm this?
>     >
>     > Hi, Richard:
>     >
>     > May I know how has the issue been fixed?
>     >
>     > I updated the tip to:
>     > ```
>     > commit 8c8ecec2a722bc2885e2648d41ac8df07bdf660d
>     >     gitsmy.py: Fix unpack of submodules of submodules
>     >     
>     >     If the submodule is in a subdirectory, it needs to have that
>     > structure
>     >     preserved.  This means the unpack path needs to be in the
>     > 'dirname' of the
>     >     final path -- since the unpack directory name is specified in the
>     > URI.
>     >     
>     >     Additional specific test cases were added to ensure this is
>     > working properly
>     >     based on two recent error reports.
>     >     
>     >     Signed-off-by: Mark Hatle <mark.hatle@windriver.com
>     <mailto:mark.hatle@windriver.com>>
>     >     Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org
>     <mailto:richard.purdie@linuxfoundation.org>
>     > >
>     > ```
>     > on top of that commit, I could see there are "git config submodule"
>     > being called concurrently in different processes, which does have a
>     > race condition, and will lead to the error observed by Stefan,
>     > although it's not easy to be reproduced.
> 
>     There are locks at a higher level in the fetchers, in
>     lib/bb/fetch2/__init__.py the download() method has:
> 
>     if ud.lockfile:
>         lf = bb.utils.lockfile(ud.lockfile)
> 
>     and the unpack() method has similar locks.
> 
>     Mark and I believe there were some code paths where the locks were
>     being bypassed in the gitsm fetcher but Mark reworked the code so that
>     we were both happy that there should always be a lock held.
> 
>     We need to fix any races properly ensuring they're covered by the main
>     fetcher locks, adding new ones would only fix a symptom of the real
>     problem.
> 
>     Cheers,
> 
>     Richard
> 
> 
>