[PATCH] sanity.bbclass: verify that user isn't building in PSEUDO_IGNORE_PATHS

[PATCH] sanity.bbclass: verify that user isn't building in PSEUDO_IGNORE_PATHS

[Dorinda]

If a user builds in a path in PSEUDO_IGNORE_PATHS, random failures are generated. Hence this patch adds a sanity check in sanity.bbclass to ensure that a user isn't building in PSEUDO_IGNORE_PATHS.

[YOCTO #14179]

Signed-off-by: Dorinda Bassey <dorindabassey@gmail.com>
---
 meta/classes/sanity.bbclass | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/meta/classes/sanity.bbclass b/meta/classes/sanity.bbclass
index 2040b48595..14271c1ca9 100644
--- a/meta/classes/sanity.bbclass
+++ b/meta/classes/sanity.bbclass
@@ -703,6 +703,13 @@ def check_sanity_version_change(status, d):
     if (tmpdirmode & stat.S_ISUID):
         status.addresult("TMPDIR is setuid, please don't build in a setuid directory")
 
+    # Check that a user isn't building in a path in PSEUDO_IGNORE_PATHS
+    pseudoignorepaths = d.getVar('PSEUDO_IGNORE_PATHS', expand=True).split(",")
+    workdir = d.getVar('WORKDIR', expand=True)
+    for i in pseudoignorepaths:
+        if i and workdir.startswith(i):
+            status.addresult("You are building in a path in PSEUDO_IGNORE_PATHS: " + str(i) + " please don't build in this directory: " + str(workdir) + "\n")
+
     # Some third-party software apparently relies on chmod etc. being suid root (!!)
     import stat
     suid_check_bins = "chown chmod mknod".split()
-- 
2.17.1

Re: [OE-core] [PATCH] sanity.bbclass: verify that user isn't building in PSEUDO_IGNORE_PATHS

[Richard Purdie]

On Tue, 2021-01-26 at 00:08 +0100, Dorinda wrote:
> If a user builds in a path in PSEUDO_IGNORE_PATHS, random failures are generated. Hence this patch adds a sanity check in sanity.bbclass to ensure that a user isn't building in PSEUDO_IGNORE_PATHS.
> 
> [YOCTO #14179]
> 
> Signed-off-by: Dorinda Bassey <dorindabassey@gmail.com>
> ---
>  meta/classes/sanity.bbclass | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/meta/classes/sanity.bbclass b/meta/classes/sanity.bbclass
> index 2040b48595..14271c1ca9 100644
> --- a/meta/classes/sanity.bbclass
> +++ b/meta/classes/sanity.bbclass
> @@ -703,6 +703,13 @@ def check_sanity_version_change(status, d):
>      if (tmpdirmode & stat.S_ISUID):
>          status.addresult("TMPDIR is setuid, please don't build in a setuid directory")
>  
> 
> 
> 
> +    # Check that a user isn't building in a path in PSEUDO_IGNORE_PATHS
> +    pseudoignorepaths = d.getVar('PSEUDO_IGNORE_PATHS', expand=True).split(",")
> +    workdir = d.getVar('WORKDIR', expand=True)
> +    for i in pseudoignorepaths:
> +        if i and workdir.startswith(i):
> +            status.addresult("You are building in a path in PSEUDO_IGNORE_PATHS: " + str(i) + " please don't build in this directory: " + str(workdir) + "\n")
> 

To ensure the user fully understands this, should this be something
like:


status.addresult("You are building in a path included in
PSEUDO_IGNORE_PATHS " + str(i) + " which will not work, please locate
the build outside this path.\n")

as we then tell the user what to do (and don't repeat effectively the
same information twice).

Cheers,

Richard

Re: [OE-core] [PATCH] sanity.bbclass: verify that user isn't building in PSEUDO_IGNORE_PATHS

[Chen Qi]

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

Hi Richard,

Is it possible to use an opposite way for pseudo path filtering?
Instead of ignoring paths, only take into consideration paths in 
something like PSEUDO_CONSIDER_PATHS.
The list should be short. ${D}, ${PKGD}, ${PKGDEST}, ${IMAGE_ROOTFS}?

Best Regards,
Chen Qi

On 01/26/2021 07:54 AM, Richard Purdie wrote:
> On Tue, 2021-01-26 at 00:08 +0100, Dorinda wrote:
>> If a user builds in a path in PSEUDO_IGNORE_PATHS, random failures are generated. Hence this patch adds a sanity check in sanity.bbclass to ensure that a user isn't building in PSEUDO_IGNORE_PATHS.
>>
>> [YOCTO #14179]
>>
>> Signed-off-by: Dorinda Bassey <dorindabassey@gmail.com>
>> ---
>>   meta/classes/sanity.bbclass | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/meta/classes/sanity.bbclass b/meta/classes/sanity.bbclass
>> index 2040b48595..14271c1ca9 100644
>> --- a/meta/classes/sanity.bbclass
>> +++ b/meta/classes/sanity.bbclass
>> @@ -703,6 +703,13 @@ def check_sanity_version_change(status, d):
>>       if (tmpdirmode & stat.S_ISUID):
>>           status.addresult("TMPDIR is setuid, please don't build in a setuid directory")
>>   
>>
>>
>>
>> +    # Check that a user isn't building in a path in PSEUDO_IGNORE_PATHS
>> +    pseudoignorepaths = d.getVar('PSEUDO_IGNORE_PATHS', expand=True).split(",")
>> +    workdir = d.getVar('WORKDIR', expand=True)
>> +    for i in pseudoignorepaths:
>> +        if i and workdir.startswith(i):
>> +            status.addresult("You are building in a path in PSEUDO_IGNORE_PATHS: " + str(i) + " please don't build in this directory: " + str(workdir) + "\n")
>>
> To ensure the user fully understands this, should this be something
> like:
>
>
> status.addresult("You are building in a path included in
> PSEUDO_IGNORE_PATHS " + str(i) + " which will not work, please locate
> the build outside this path.\n")
>
> as we then tell the user what to do (and don't repeat effectively the
> same information twice).
>
> Cheers,
>
> Richard
>
>
>
> 
>


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

Re: [OE-core] [PATCH] sanity.bbclass: verify that user isn't building in PSEUDO_IGNORE_PATHS

[Richard Purdie]

Hi,

On Wed, 2021-01-27 at 17:46 +0800, ChenQi wrote:
>  Is it possible to use an opposite way for pseudo path filtering?
>  Instead of ignoring paths, only take into consideration paths in
> something like PSEUDO_CONSIDER_PATHS.
>  The list should be short. ${D}, ${PKGD}, ${PKGDEST},
> ${IMAGE_ROOTFS}?

Its a good question, I was convinced by the pseudo authors that it was
a bad idea. The reason being that it isn't just the above paths but we
also need to catch calls where files are transiently in other
directories or in other parts of the filesystem such as /tmp/. Code
creates files in interesting and convoluted ways to preserve
permissions, for security reasons and so on and there was a worry that
masking just a specific list would cause a different set of problems.
It probably shouldn't be crossing filesystem boundaries doing it but I
know there were complications due to it.

That said, now that pseudo can support path filtering, it may be worth
an experiment to see how whether it could work better than what we're
doing now. The algorithm (whether its include or exclude) should be
relatively easy to test.

I can also say the above list would need expanding as some sstate
objects need to preserve ownership/permissions during creating and so
on. It probably would still be simpler than our current ignore list
though.

Cheers,

Richard 

Re: [OE-core] [PATCH] sanity.bbclass: verify that user isn't building in PSEUDO_IGNORE_PATHS

[Chen Qi]

On 01/27/2021 05:49 PM, Richard Purdie wrote:
> Hi,
>
> On Wed, 2021-01-27 at 17:46 +0800, ChenQi wrote:
>>   Is it possible to use an opposite way for pseudo path filtering?
>>   Instead of ignoring paths, only take into consideration paths in
>> something like PSEUDO_CONSIDER_PATHS.
>>   The list should be short. ${D}, ${PKGD}, ${PKGDEST},
>> ${IMAGE_ROOTFS}?
> Its a good question, I was convinced by the pseudo authors that it was
> a bad idea. The reason being that it isn't just the above paths but we
> also need to catch calls where files are transiently in other
> directories or in other parts of the filesystem such as /tmp/. Code
> creates files in interesting and convoluted ways to preserve
> permissions, for security reasons and so on and there was a worry that
> masking just a specific list would cause a different set of problems.
> It probably shouldn't be crossing filesystem boundaries doing it but I
> know there were complications due to it.
>
> That said, now that pseudo can support path filtering, it may be worth
> an experiment to see how whether it could work better than what we're
> doing now. The algorithm (whether its include or exclude) should be
> relatively easy to test.

Yes. I'm now testing the patch.

>
> I can also say the above list would need expanding as some sstate
> objects need to preserve ownership/permissions during creating and so
> on.

Thanks. You are right. The current list is 
"${D},${PKGD},${PKGDEST},${IMAGE_ROOTFS},${SDK_OUTPUT},${STAGING_DIR}"

I'm now doing more build testing to see if it needs to be expanded.

Best Regards,
Chen Qi

> It probably would still be simpler than our current ignore list
> though.
>
> Cheers,
>
> Richard
>
>
>
>
>

Re: [OE-core] [PATCH] sanity.bbclass: verify that user isn't building in PSEUDO_IGNORE_PATHS

[Chen Qi]

Hi Richard,

I've sent out the patches. They are tested with:
1. bitbake IMAGE && bitbake IMAGE -c populate_sdk
2. bitbake world
3. bitbake IMAGE -c populate_sdk_ext; Install the eSDK

The world build covers all oe-core recipes and part of the recipes in 
some other meta layers such as meta-openembedded, meta-virtualization, 
meta-secure-core, etc. The recipe number is about 2000.

Best Regards,
Chen Qi

On 01/27/2021 05:49 PM, Richard Purdie wrote:
> Hi,
>
> On Wed, 2021-01-27 at 17:46 +0800, ChenQi wrote:
>>   Is it possible to use an opposite way for pseudo path filtering?
>>   Instead of ignoring paths, only take into consideration paths in
>> something like PSEUDO_CONSIDER_PATHS.
>>   The list should be short. ${D}, ${PKGD}, ${PKGDEST},
>> ${IMAGE_ROOTFS}?
> Its a good question, I was convinced by the pseudo authors that it was
> a bad idea. The reason being that it isn't just the above paths but we
> also need to catch calls where files are transiently in other
> directories or in other parts of the filesystem such as /tmp/. Code
> creates files in interesting and convoluted ways to preserve
> permissions, for security reasons and so on and there was a worry that
> masking just a specific list would cause a different set of problems.
> It probably shouldn't be crossing filesystem boundaries doing it but I
> know there were complications due to it.
>
> That said, now that pseudo can support path filtering, it may be worth
> an experiment to see how whether it could work better than what we're
> doing now. The algorithm (whether its include or exclude) should be
> relatively easy to test.
>
> I can also say the above list would need expanding as some sstate
> objects need to preserve ownership/permissions during creating and so
> on. It probably would still be simpler than our current ignore list
> though.
>
> Cheers,
>
> Richard
>
>
>
>
>

Re: [OE-core] [PATCH] sanity.bbclass: verify that user isn't building in PSEUDO_IGNORE_PATHS

[Quentin Schulz]

Hi Dorinda,

On Tue, Jan 26, 2021 at 01:53:47AM +0100, Dorinda wrote:
> If a user builds in a path in PSEUDO_IGNORE_PATHS, random failures are generated. Hence this patch adds a sanity check in sanity.bbclass to ensure that a user isn't building in PSEUDO_IGNORE_PATHS.
> 

Commit log is usually wraped when reaching a given length. I personally
have it set to 72 characters which IIRC is what's used for kernel
contribution.

This does not apply to actual logs provided in the commit log.

It's just nitpick, as I saw a commit or two not following this implicit
"rule" in poky, I'd say there's no need to resend a patch just for this.

Also, please add a version tag when you send revamped patches.

You can do that by adding -v 2 to git format-patch.

> [YOCTO #14179]
> 
> Signed-off-by: Dorinda Bassey <dorindabassey@gmail.com>
> ---

And if you want to do it really the very nice way (IMO, as always), you
can provide below the --- (the one above this) and the list of files
changed (the one below this), you can list what has changed between v
N-1 and v N.

e.g.

 v2:
  - rephrased error log,
  - added a check for empty paths in pseudoignorepaths,

>  meta/classes/sanity.bbclass | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/meta/classes/sanity.bbclass b/meta/classes/sanity.bbclass
> index e527c13746..c6842ff549 100644
> --- a/meta/classes/sanity.bbclass
> +++ b/meta/classes/sanity.bbclass

Only nitpicks, so nothing to be done on my side if no new change is
requested by other reviewers.

Cheers,
Quentin