Re: [oe-commits] Alex Franco : Fix mode +st on TMPDIR when creating it

Re: [oe-commits] Alex Franco : Fix mode +st on TMPDIR when creating it

[Martin Jansa]

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

On Tue, Sep 01, 2015 at 10:45:22AM +0000, git@git.openembedded.org wrote:
> Module: openembedded-core.git
> Branch: master
> Commit: 8236d57439640a185c0226312cd4903a3ce2f53b
> URL:    http://git.openembedded.org/?p=openembedded-core.git&a=commit;h=8236d57439640a185c0226312cd4903a3ce2f53b
> 
> Author: Alex Franco <alejandro.franco@linux.intel.com>
> Date:   Fri Aug 28 17:34:04 2015 -0500
> 
> Fix mode +st on TMPDIR when creating it
> 
> A sanity check fails when TMPDIR has setuid, setgid set. It was
> proposed to fix this on TMPDIR creation instead of failing with
> a sanity test only. This patch handles removal of those special
> bits (and additonally, the sticky bit) from BUILDDIR and TMPDIR
> when these directories are created.

Can you explain why it's needed to remove them?

I have tmp-glibc mounted as tmpfs:
drwxrwxrwt   4 root    jenkins   80 Sep  1 19:02 tmp-glibc

and since this change all bitbake executions are failing with:

NOTE: Started PRServer with DBfile: /home/jenkins/oe/world/shr-core/cache/prserv.sqlite3, IP: 127.0.0.1, PORT: 43102, PID: 9220
ERROR: Execution of event handler 'check_sanity_eventhandler' failed
Traceback (most recent call last):
  File "check_sanity_eventhandler(e)", line 6, in check_sanity_eventhandler(e=<bb.event.SanityCheck object at 0x3c76ad0>)
  File "sanity.bbclass", line 34, in check_sanity(sanity_data=<bb.data_smart.DataSmart object at 0x3c83890>)
  File "sanity.bbclass", line 157, in check_sanity_everybuild(status=<SanityStatus object at 0x3c83850>, d=<bb.data_smart.DataSmart object at 0x3c83890>)
OSError: [Errno 1] Operation not permitted: '/home/jenkins/oe/world/shr-core/tmp-glibc'

I can change the permissions manually, but I would like to know
why is this needed and checked every time the bitbake is executed.

Regards,

> 
> [YOCTO #7669]
> 
> Signed-off-by: Alex Franco <alejandro.franco@linux.intel.com>
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> 
> ---
> 
>  meta/classes/sanity.bbclass | 5 +++++
>  scripts/oe-setup-builddir   | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/meta/classes/sanity.bbclass b/meta/classes/sanity.bbclass
> index ef90fc8..2864318 100644
> --- a/meta/classes/sanity.bbclass
> +++ b/meta/classes/sanity.bbclass
> @@ -686,6 +686,7 @@ def check_sanity_version_change(status, d):
>      status.addresult(check_not_nfs(tmpdir, "TMPDIR"))
>  
>  def check_sanity_everybuild(status, d):
> +    import os, stat
>      # Sanity tests which test the users environment so need to run at each build (or are so cheap
>      # it makes sense to always run them.
>  
> @@ -839,6 +840,10 @@ def check_sanity_everybuild(status, d):
>                  status.addresult("Error, TMPDIR has changed location. You need to either move it back to %s or rebuild\n" % saved_tmpdir)
>      else:
>          bb.utils.mkdirhier(tmpdir)
> +        # Remove setuid, setgid and sticky bits from TMPDIR
> +        os.chmod(tmpdir, os.stat(tmpdir).st_mode & ~ stat.S_ISUID)
> +        os.chmod(tmpdir, os.stat(tmpdir).st_mode & ~ stat.S_ISGID)
> +        os.chmod(tmpdir, os.stat(tmpdir).st_mode & ~ stat.S_ISVTX)
>          with open(checkfile, "w") as f:
>              f.write(tmpdir)
>  
> diff --git a/scripts/oe-setup-builddir b/scripts/oe-setup-builddir
> index add0b50..f5b7e4e 100755
> --- a/scripts/oe-setup-builddir
> +++ b/scripts/oe-setup-builddir
> @@ -24,6 +24,7 @@ if [ -z "$BUILDDIR" ]; then
>  fi
>  
>  mkdir -p "$BUILDDIR/conf"
> +chmod -R -st "$BUILDDIR" 
>  
>  if [ ! -d "$BUILDDIR" ]; then
>      echo >&2 "Error: The builddir ($BUILDDIR) does not exist!"
> 
> -- 
> _______________________________________________
> Openembedded-commits mailing list
> Openembedded-commits@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-commits

-- 
Martin 'JaMa' Jansa     jabber: Martin.Jansa@gmail.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 188 bytes --]

Re: [oe-commits] Alex Franco : Fix mode +st on TMPDIR when creating it

[Alex Franco]

Hi Martin,

I gather the original reason (for the sanity test, as well as these 
changes) is that it is better for everything under BUILDDIR - and 
certainly TMPDIR to be owned by the user running bitbake and have 
standard permissions, the sticky bit itself would prevent a user running 
bitbake, even with +rwx permissions, from making changes to items it 
might need to manipulate, if those are not owned by this user. Similarly 
setuid and setgid might propagate user/group ownerships different from 
the user bitbake is running as, into BUILDDIR and TMPDIR (and items 
therein).

For more info take a look at: 
https://bugzilla.yoctoproject.org/show_bug.cgi?id=7669

Perhaps there are more pressing reasons, perhaps Christopher Larson can 
comment a bit on this.

Alex Franco

On 09/01/2015 02:09 PM, Martin Jansa wrote:
> On Tue, Sep 01, 2015 at 10:45:22AM +0000, git@git.openembedded.org wrote:
>> Module: openembedded-core.git
>> Branch: master
>> Commit: 8236d57439640a185c0226312cd4903a3ce2f53b
>> URL:    http://git.openembedded.org/?p=openembedded-core.git&a=commit;h=8236d57439640a185c0226312cd4903a3ce2f53b
>>
>> Author: Alex Franco <alejandro.franco@linux.intel.com>
>> Date:   Fri Aug 28 17:34:04 2015 -0500
>>
>> Fix mode +st on TMPDIR when creating it
>>
>> A sanity check fails when TMPDIR has setuid, setgid set. It was
>> proposed to fix this on TMPDIR creation instead of failing with
>> a sanity test only. This patch handles removal of those special
>> bits (and additonally, the sticky bit) from BUILDDIR and TMPDIR
>> when these directories are created.
> Can you explain why it's needed to remove them?
>
> I have tmp-glibc mounted as tmpfs:
> drwxrwxrwt   4 root    jenkins   80 Sep  1 19:02 tmp-glibc
>
> and since this change all bitbake executions are failing with:
>
> NOTE: Started PRServer with DBfile: /home/jenkins/oe/world/shr-core/cache/prserv.sqlite3, IP: 127.0.0.1, PORT: 43102, PID: 9220
> ERROR: Execution of event handler 'check_sanity_eventhandler' failed
> Traceback (most recent call last):
>    File "check_sanity_eventhandler(e)", line 6, in check_sanity_eventhandler(e=<bb.event.SanityCheck object at 0x3c76ad0>)
>    File "sanity.bbclass", line 34, in check_sanity(sanity_data=<bb.data_smart.DataSmart object at 0x3c83890>)
>    File "sanity.bbclass", line 157, in check_sanity_everybuild(status=<SanityStatus object at 0x3c83850>, d=<bb.data_smart.DataSmart object at 0x3c83890>)
> OSError: [Errno 1] Operation not permitted: '/home/jenkins/oe/world/shr-core/tmp-glibc'
>
> I can change the permissions manually, but I would like to know
> why is this needed and checked every time the bitbake is executed.
>
> Regards,
>
>> [YOCTO #7669]
>>
>> Signed-off-by: Alex Franco <alejandro.franco@linux.intel.com>
>> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
>>
>> ---
>>
>>   meta/classes/sanity.bbclass | 5 +++++
>>   scripts/oe-setup-builddir   | 1 +
>>   2 files changed, 6 insertions(+)
>>
>> diff --git a/meta/classes/sanity.bbclass b/meta/classes/sanity.bbclass
>> index ef90fc8..2864318 100644
>> --- a/meta/classes/sanity.bbclass
>> +++ b/meta/classes/sanity.bbclass
>> @@ -686,6 +686,7 @@ def check_sanity_version_change(status, d):
>>       status.addresult(check_not_nfs(tmpdir, "TMPDIR"))
>>   
>>   def check_sanity_everybuild(status, d):
>> +    import os, stat
>>       # Sanity tests which test the users environment so need to run at each build (or are so cheap
>>       # it makes sense to always run them.
>>   
>> @@ -839,6 +840,10 @@ def check_sanity_everybuild(status, d):
>>                   status.addresult("Error, TMPDIR has changed location. You need to either move it back to %s or rebuild\n" % saved_tmpdir)
>>       else:
>>           bb.utils.mkdirhier(tmpdir)
>> +        # Remove setuid, setgid and sticky bits from TMPDIR
>> +        os.chmod(tmpdir, os.stat(tmpdir).st_mode & ~ stat.S_ISUID)
>> +        os.chmod(tmpdir, os.stat(tmpdir).st_mode & ~ stat.S_ISGID)
>> +        os.chmod(tmpdir, os.stat(tmpdir).st_mode & ~ stat.S_ISVTX)
>>           with open(checkfile, "w") as f:
>>               f.write(tmpdir)
>>   
>> diff --git a/scripts/oe-setup-builddir b/scripts/oe-setup-builddir
>> index add0b50..f5b7e4e 100755
>> --- a/scripts/oe-setup-builddir
>> +++ b/scripts/oe-setup-builddir
>> @@ -24,6 +24,7 @@ if [ -z "$BUILDDIR" ]; then
>>   fi
>>   
>>   mkdir -p "$BUILDDIR/conf"
>> +chmod -R -st "$BUILDDIR"
>>   
>>   if [ ! -d "$BUILDDIR" ]; then
>>       echo >&2 "Error: The builddir ($BUILDDIR) does not exist!"
>>
>> -- 
>> _______________________________________________
>> Openembedded-commits mailing list
>> Openembedded-commits@lists.openembedded.org
>> http://lists.openembedded.org/mailman/listinfo/openembedded-commits

Re: [oe-commits] Alex Franco : Fix mode +st on TMPDIR when creating it

[Christopher Larson]

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

On Tue, Sep 1, 2015 at 4:11 PM, Alex Franco <
alejandro.franco@linux.intel.com> wrote:

> I gather the original reason (for the sanity test, as well as these
> changes) is that it is better for everything under BUILDDIR - and certainly
> TMPDIR to be owned by the user running bitbake and have standard
> permissions, the sticky bit itself would prevent a user running bitbake,
> even with +rwx permissions, from making changes to items it might need to
> manipulate, if those are not owned by this user. Similarly setuid and
> setgid might propagate user/group ownerships different from the user
> bitbake is running as, into BUILDDIR and TMPDIR (and items therein).
>
> For more info take a look at:
> https://bugzilla.yoctoproject.org/show_bug.cgi?id=7669
>
> Perhaps there are more pressing reasons, perhaps Christopher Larson can
> comment a bit on this.
>

Indeed, the g+s can flow down into the files put into the packages, and
then into the rootfs. I ran into a this problem quite a bit, since my
project areas are in a g+s area to keep the files accessible to the group.
-- 
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics

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

Re: [oe-commits] Alex Franco : Fix mode +st on TMPDIR when creating it

[Richard Purdie]

On Tue, 2015-09-01 at 16:47 -0700, Christopher Larson wrote:
> 
> On Tue, Sep 1, 2015 at 4:11 PM, Alex Franco
> <alejandro.franco@linux.intel.com> wrote:
>         I gather the original reason (for the sanity test, as well as
>         these changes) is that it is better for everything under
>         BUILDDIR - and certainly TMPDIR to be owned by the user
>         running bitbake and have standard permissions, the sticky bit
>         itself would prevent a user running bitbake, even with +rwx
>         permissions, from making changes to items it might need to
>         manipulate, if those are not owned by this user. Similarly
>         setuid and setgid might propagate user/group ownerships
>         different from the user bitbake is running as, into BUILDDIR
>         and TMPDIR (and items therein).
>         
>         For more info take a look at:
>         https://bugzilla.yoctoproject.org/show_bug.cgi?id=7669
>         
>         Perhaps there are more pressing reasons, perhaps Christopher
>         Larson can comment a bit on this.
> 
> Indeed, the g+s can flow down into the files put into the packages,
> and then into the rootfs. I ran into a this problem quite a bit, since
> my project areas are in a g+s area to keep the files accessible to the
> group.


There is actually another problem here. The -R means if you do this to
an existing build directory, it takes around 10 mins on the machine I
just rebooted with a cold cache. We need to lose the -R there.

Cheers,

Richard

Re: [oe-commits] Alex Franco : Fix mode +st on TMPDIR when creating it

[Alex Franco]

Thanks, I have sent a patch after removing the -R and testing a bit

Alex Franco

On 09/02/2015 06:14 AM, Richard Purdie wrote:
> On Tue, 2015-09-01 at 16:47 -0700, Christopher Larson wrote:
>> On Tue, Sep 1, 2015 at 4:11 PM, Alex Franco
>> <alejandro.franco@linux.intel.com> wrote:
>>          I gather the original reason (for the sanity test, as well as
>>          these changes) is that it is better for everything under
>>          BUILDDIR - and certainly TMPDIR to be owned by the user
>>          running bitbake and have standard permissions, the sticky bit
>>          itself would prevent a user running bitbake, even with +rwx
>>          permissions, from making changes to items it might need to
>>          manipulate, if those are not owned by this user. Similarly
>>          setuid and setgid might propagate user/group ownerships
>>          different from the user bitbake is running as, into BUILDDIR
>>          and TMPDIR (and items therein).
>>          
>>          For more info take a look at:
>>          https://bugzilla.yoctoproject.org/show_bug.cgi?id=7669
>>          
>>          Perhaps there are more pressing reasons, perhaps Christopher
>>          Larson can comment a bit on this.
>>
>> Indeed, the g+s can flow down into the files put into the packages,
>> and then into the rootfs. I ran into a this problem quite a bit, since
>> my project areas are in a g+s area to keep the files accessible to the
>> group.
>
> There is actually another problem here. The -R means if you do this to
> an existing build directory, it takes around 10 mins on the machine I
> just rebooted with a cold cache. We need to lose the -R there.
>
> Cheers,
>
> Richard
>
>

Re: [oe-commits] Alex Franco : Fix mode +st on TMPDIR when creating it

[Richard Purdie]

On Tue, 2015-09-01 at 21:09 +0200, Martin Jansa wrote:
> On Tue, Sep 01, 2015 at 10:45:22AM +0000, git@git.openembedded.org wrote:
> > Module: openembedded-core.git
> > Branch: master
> > Commit: 8236d57439640a185c0226312cd4903a3ce2f53b
> > URL:    http://git.openembedded.org/?p=openembedded-core.git&a=commit;h=8236d57439640a185c0226312cd4903a3ce2f53b
> > 
> > Author: Alex Franco <alejandro.franco@linux.intel.com>
> > Date:   Fri Aug 28 17:34:04 2015 -0500
> > 
> > Fix mode +st on TMPDIR when creating it
> > 
> > A sanity check fails when TMPDIR has setuid, setgid set. It was
> > proposed to fix this on TMPDIR creation instead of failing with
> > a sanity test only. This patch handles removal of those special
> > bits (and additonally, the sticky bit) from BUILDDIR and TMPDIR
> > when these directories are created.
> 
> Can you explain why it's needed to remove them?

At least in theory the sticky bit (t) can end up influencing the target
package file ownership. In the interests of build determinism we really
need TMPDIR to start in a consistent way with a consistent set of
permissions.

Put another way, I don't really want to have to add new sanity tests
that test if builds work the same if TMPDIR is +t compared to a dir that
isn't set that way.

So if you could tweak your autobuilder setup to lose the sticky bit on
that directory, I think that would be preferable to adding more corner
cases to the build environment.

If everyone had setups like this, that would change my view but it
doesn't seem to eb the ase.

Cheers,

Richard