[bitbake-devel] [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 at linuxfoundation.org
> <mailto:richard.purdie at 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 at windriver.com
>     <mailto:mark.hatle at windriver.com>>
>     >     Signed-off-by: Richard Purdie <richard.purdie at linuxfoundation.org
>     <mailto:richard.purdie at 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
> 
> 
>