[PATCH v2] build: defer processing of [dirs] and [cleandirs] until after lockfiles are acquired

[PATCH v2] build: defer processing of [dirs] and [cleandirs] until after lockfiles are acquired

[Chris Laplante]

This is to avoid potential races for tasks that use both [cleandirs] and
[lockfiles].

Note that [dirs] cannot be processed until after [cleandirs], since
the two may intersect, e.g.:

    do_task[cleandirs] += "${WORKDIR}/my_clean_dir"
    do_task[dirs] =+ "${WORKDIR}/my_clean_dir/subdir"

This patch also adds sanity checking to detect attempts to create
lockfiles under directories listed in [cleandirs] which, as of this
patch, would make no sense.

Signed-off-by: Chris Laplante <chris.laplante@agilent.com>
---
 lib/bb/build.py | 47 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/lib/bb/build.py b/lib/bb/build.py
index 974d2ff0..d3a43778 100644
--- a/lib/bb/build.py
+++ b/lib/bb/build.py
@@ -179,7 +179,7 @@ class StdoutNoopContextManager:
         return sys.stdout.name
 
 
-def exec_func(func, d, dirs = None):
+def exec_func(func, d, dirs=None):
     """Execute a BB 'function'"""
 
     try:
@@ -188,24 +188,11 @@ def exec_func(func, d, dirs = None):
         oldcwd = None
 
     flags = d.getVarFlags(func)
-    cleandirs = flags.get('cleandirs') if flags else None
-    if cleandirs:
-        for cdir in d.expand(cleandirs).split():
-            bb.utils.remove(cdir, True)
-            bb.utils.mkdirhier(cdir)
-
     if flags and dirs is None:
         dirs = flags.get('dirs')
         if dirs:
             dirs = d.expand(dirs).split()
 
-    if dirs:
-        for adir in dirs:
-            bb.utils.mkdirhier(adir)
-        adir = dirs[-1]
-    else:
-        adir = None
-
     body = d.getVar(func, False)
     if not body:
         if body is None:
@@ -249,7 +236,39 @@ def exec_func(func, d, dirs = None):
             except OSError:
                 pass
 
+    cleandirs = flags.get('cleandirs') if flags else None
+    cleandirs = d.expand(cleandirs).split() if cleandirs else []
+
+    if cleandirs and lockfiles:
+        # Sanity check that no lockfiles are under any of the cleandirs.
+        # Since cleandirs is processed after taking the lockfiles, we want to guard against deleting the lockfile's
+        # enclosing directories.
+        import pathlib
+        for lockfile in lockfiles:
+            lockfile_path = pathlib.PosixPath(lockfile).resolve()
+            for cleandir in cleandirs:
+                cleandir_path = pathlib.PosixPath(cleandir).resolve()
+                try:
+                    lockfile_path.relative_to(cleandir_path)
+                except ValueError:
+                    continue
+                bb.fatal("Lockfile {0} cannot be placed inside {1} because that directory is set to be cleaned (see "
+                         "[cleandirs] flag)".format(lockfile_path, cleandir_path))
+
     with bb.utils.fileslocked(lockfiles):
+        # Process [cleandirs] varflag
+        for cdir in cleandirs:
+            bb.utils.remove(cdir, True)
+            bb.utils.mkdirhier(cdir)
+
+        # Now process [dirs] varflag
+        if dirs:
+            for adir in dirs:
+                bb.utils.mkdirhier(adir)
+            adir = dirs[-1]
+        else:
+            adir = None
+
         if ispython:
             exec_func_python(func, d, runfile, cwd=adir)
         else:
-- 
2.17.1

Re: [bitbake-devel] [PATCH v2] build: defer processing of [dirs] and [cleandirs] until after lockfiles are acquired

[Ross Burton]

This is causing failures when building kernel-devsrc:
http://errors.yoctoproject.org/Errors/Details/537939/

Ross

On Mon, 9 Nov 2020 at 18:56, Chris Laplante via lists.openembedded.org
<chris.laplante=agilent.com@lists.openembedded.org> wrote:
>
> This is to avoid potential races for tasks that use both [cleandirs] and
> [lockfiles].
>
> Note that [dirs] cannot be processed until after [cleandirs], since
> the two may intersect, e.g.:
>
>     do_task[cleandirs] += "${WORKDIR}/my_clean_dir"
>     do_task[dirs] =+ "${WORKDIR}/my_clean_dir/subdir"
>
> This patch also adds sanity checking to detect attempts to create
> lockfiles under directories listed in [cleandirs] which, as of this
> patch, would make no sense.
>
> Signed-off-by: Chris Laplante <chris.laplante@agilent.com>
> ---
>  lib/bb/build.py | 47 +++++++++++++++++++++++++++++++++--------------
>  1 file changed, 33 insertions(+), 14 deletions(-)
>
> diff --git a/lib/bb/build.py b/lib/bb/build.py
> index 974d2ff0..d3a43778 100644
> --- a/lib/bb/build.py
> +++ b/lib/bb/build.py
> @@ -179,7 +179,7 @@ class StdoutNoopContextManager:
>          return sys.stdout.name
>
>
> -def exec_func(func, d, dirs = None):
> +def exec_func(func, d, dirs=None):
>      """Execute a BB 'function'"""
>
>      try:
> @@ -188,24 +188,11 @@ def exec_func(func, d, dirs = None):
>          oldcwd = None
>
>      flags = d.getVarFlags(func)
> -    cleandirs = flags.get('cleandirs') if flags else None
> -    if cleandirs:
> -        for cdir in d.expand(cleandirs).split():
> -            bb.utils.remove(cdir, True)
> -            bb.utils.mkdirhier(cdir)
> -
>      if flags and dirs is None:
>          dirs = flags.get('dirs')
>          if dirs:
>              dirs = d.expand(dirs).split()
>
> -    if dirs:
> -        for adir in dirs:
> -            bb.utils.mkdirhier(adir)
> -        adir = dirs[-1]
> -    else:
> -        adir = None
> -
>      body = d.getVar(func, False)
>      if not body:
>          if body is None:
> @@ -249,7 +236,39 @@ def exec_func(func, d, dirs = None):
>              except OSError:
>                  pass
>
> +    cleandirs = flags.get('cleandirs') if flags else None
> +    cleandirs = d.expand(cleandirs).split() if cleandirs else []
> +
> +    if cleandirs and lockfiles:
> +        # Sanity check that no lockfiles are under any of the cleandirs.
> +        # Since cleandirs is processed after taking the lockfiles, we want to guard against deleting the lockfile's
> +        # enclosing directories.
> +        import pathlib
> +        for lockfile in lockfiles:
> +            lockfile_path = pathlib.PosixPath(lockfile).resolve()
> +            for cleandir in cleandirs:
> +                cleandir_path = pathlib.PosixPath(cleandir).resolve()
> +                try:
> +                    lockfile_path.relative_to(cleandir_path)
> +                except ValueError:
> +                    continue
> +                bb.fatal("Lockfile {0} cannot be placed inside {1} because that directory is set to be cleaned (see "
> +                         "[cleandirs] flag)".format(lockfile_path, cleandir_path))
> +
>      with bb.utils.fileslocked(lockfiles):
> +        # Process [cleandirs] varflag
> +        for cdir in cleandirs:
> +            bb.utils.remove(cdir, True)
> +            bb.utils.mkdirhier(cdir)
> +
> +        # Now process [dirs] varflag
> +        if dirs:
> +            for adir in dirs:
> +                bb.utils.mkdirhier(adir)
> +            adir = dirs[-1]
> +        else:
> +            adir = None
> +
>          if ispython:
>              exec_func_python(func, d, runfile, cwd=adir)
>          else:
> --
> 2.17.1
>
>
> 
>