GNU bug report logs - #16335
Segmentation fault when using cp -a with SELinux and fakeroot

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 16335 in the body.
You can then email your comments to 16335 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Nicolas Iooss <nicolas.iooss <at> m4x.org>
To: bug-coreutils <at> gnu.org
Subject: Segmentation fault when using cp -a with SELinux and fakeroot
Date: Fri, 03 Jan 2014 23:08:42 +0100

[Message part 1 (text/plain, inline)]

Hello,

After upgrading to coreutils 8.22 I can no longer build packages which
uses "cp -a" to copy files due to a segmentation fault happening in
libselinux.

I've tried to reproduce this bug with few commands, in a directory which
doesn't have any default context:

    $ mkdir /tmp/foobar
    $ matchpathcon
    /tmp/foobar	<<none>>
    $ touch /tmp/foobar/a
    $ fakeroot cp -a /tmp/foobar/a /tmp/foobar/b
    $ fakeroot cp -a /tmp/foobar/a /tmp/foobar/b
    /usr/bin/fakeroot: line 181:  9207 Segmentation fault

Without fakeroot there is no segmentation fault.

Even if the message says "/usr/bin/fakeroot", a coredump has been
created for cp. I've analyzed this dump using gdb and after some
debugging, I found out that restorecon_private (from src/selinux.c) was
calling lsetfilecon with a NULL security context which was obtained by
getfscreatecon (case "local = true" in the code [1]). This causes a null
pointer dereference in libselinux and so a SIGSEGV.

I've reported this bug to libselinux maintainers [2] and got the reply
that calling lsetfilecon with a NULL security context was like calling
strlen with a NULL string and that this was a problem in caller's code [3].

Hence I propose the attached patch to fix the segmentation fault. Could
you please accept it?

When you reply, please Cc me as I'm not subscribed.

Thanks,

Nicolas Iooss

-----------

System configuration during my tests:

* distro: ArchLinux which SELinux packages
* CPU arch: x86_64
* SELinux in permissive mode
* coreutils 8.22
* libselinux 2.2.1
* fakeroot 1.20

[1]
http://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=blob;f=src/selinux.c;hb=v8.22#l191
[2] http://marc.info/?l=selinux&m=138763485330568&w=2
[3] http://marc.info/?l=selinux&m=138842015508829&w=2

[0001-Fix-segmentation-fault-in-restorecon_private.patch (text/x-patch, attachment)]

Message #8 received at 16335 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Nicolas Iooss <nicolas.iooss <at> m4x.org>
Cc: 16335 <at> debbugs.gnu.org
Subject: Re: bug#16335: Segmentation fault when using cp -a with SELinux and
 fakeroot
Date: Sat, 04 Jan 2014 01:42:01 +0000

On 01/03/2014 10:08 PM, Nicolas Iooss wrote:
> Hello,
> 
> After upgrading to coreutils 8.22 I can no longer build packages which
> uses "cp -a" to copy files due to a segmentation fault happening in
> libselinux.
> 
> I've tried to reproduce this bug with few commands, in a directory which
> doesn't have any default context:
> 
>     $ mkdir /tmp/foobar
>     $ matchpathcon
>     /tmp/foobar	<<none>>
>     $ touch /tmp/foobar/a
>     $ fakeroot cp -a /tmp/foobar/a /tmp/foobar/b
>     $ fakeroot cp -a /tmp/foobar/a /tmp/foobar/b
>     /usr/bin/fakeroot: line 181:  9207 Segmentation fault
> 
> Without fakeroot there is no segmentation fault.
> 
> Even if the message says "/usr/bin/fakeroot", a coredump has been
> created for cp. I've analyzed this dump using gdb and after some
> debugging, I found out that restorecon_private (from src/selinux.c) was
> calling lsetfilecon with a NULL security context which was obtained by
> getfscreatecon (case "local = true" in the code [1]). This causes a null
> pointer dereference in libselinux and so a SIGSEGV.
> 
> I've reported this bug to libselinux maintainers [2] and got the reply
> that calling lsetfilecon with a NULL security context was like calling
> strlen with a NULL string and that this was a problem in caller's code [3].
> 
> Hence I propose the attached patch to fix the segmentation fault. Could
> you please accept it?
> 
> When you reply, please Cc me as I'm not subscribed.
> 
> Thanks,
> 
> Nicolas Iooss
> 
> -----------
> 
> System configuration during my tests:
> 
> * distro: ArchLinux which SELinux packages
> * CPU arch: x86_64
> * SELinux in permissive mode
> * coreutils 8.22
> * libselinux 2.2.1
> * fakeroot 1.20
> 
> [1]
> http://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=blob;f=src/selinux.c;hb=v8.22#l191
> [2] http://marc.info/?l=selinux&m=138763485330568&w=2
> [3] http://marc.info/?l=selinux&m=138842015508829&w=2

Thanks for the very thorough analysis and patch.
The patch looks correct as getfscreatecon() is
documented to return a NULL context in some cases.
I'll see if I can add a robust test and will apply
this in your name.

thanks,
Pádraig.

Message #11 received at 16335 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Nicolas Iooss <nicolas.iooss <at> m4x.org>
Cc: 16335 <at> debbugs.gnu.org
Subject: Re: bug#16335: Segmentation fault when using cp -a with SELinux and
 fakeroot
Date: Sat, 04 Jan 2014 03:03:06 +0000

On 01/04/2014 01:42 AM, Pádraig Brady wrote:
> On 01/03/2014 10:08 PM, Nicolas Iooss wrote:
>> Hello,
>>
>> After upgrading to coreutils 8.22 I can no longer build packages which
>> uses "cp -a" to copy files due to a segmentation fault happening in
>> libselinux.
>>
>> I've tried to reproduce this bug with few commands, in a directory which
>> doesn't have any default context:
>>
>>     $ mkdir /tmp/foobar
>>     $ matchpathcon
>>     /tmp/foobar	<<none>>
>>     $ touch /tmp/foobar/a
>>     $ fakeroot cp -a /tmp/foobar/a /tmp/foobar/b
>>     $ fakeroot cp -a /tmp/foobar/a /tmp/foobar/b
>>     /usr/bin/fakeroot: line 181:  9207 Segmentation fault
>>
>> Without fakeroot there is no segmentation fault.
>>
>> Even if the message says "/usr/bin/fakeroot", a coredump has been
>> created for cp. I've analyzed this dump using gdb and after some
>> debugging, I found out that restorecon_private (from src/selinux.c) was
>> calling lsetfilecon with a NULL security context which was obtained by
>> getfscreatecon (case "local = true" in the code [1]). This causes a null
>> pointer dereference in libselinux and so a SIGSEGV.
>>
>> I've reported this bug to libselinux maintainers [2] and got the reply
>> that calling lsetfilecon with a NULL security context was like calling
>> strlen with a NULL string and that this was a problem in caller's code [3].
>>
>> Hence I propose the attached patch to fix the segmentation fault. Could
>> you please accept it?
>>
>> When you reply, please Cc me as I'm not subscribed.
>>
>> Thanks,
>>
>> Nicolas Iooss
>>
>> -----------
>>
>> System configuration during my tests:
>>
>> * distro: ArchLinux which SELinux packages
>> * CPU arch: x86_64
>> * SELinux in permissive mode
>> * coreutils 8.22
>> * libselinux 2.2.1
>> * fakeroot 1.20
>>
>> [1]
>> http://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=blob;f=src/selinux.c;hb=v8.22#l191
>> [2] http://marc.info/?l=selinux&m=138763485330568&w=2
>> [3] http://marc.info/?l=selinux&m=138842015508829&w=2
> 
> Thanks for the very thorough analysis and patch.
> The patch looks correct as getfscreatecon() is
> documented to return a NULL context in some cases.
> I'll see if I can add a robust test and will apply
> this in your name.

Actually what's errno set to with tcon is NULL.
If if was 0 you might get the classic "error success" message
if using the --preserve=context option rather than -a for example.

I.E. the following might be more appropriate.
Note neither Fedora 15 or 20 here produce a NULL value with fakeroot.

thanks,
Pádraig.

diff --git a/src/selinux.c b/src/selinux.c
index cd38a81..016db16 100644
--- a/src/selinux.c
+++ b/src/selinux.c
@@ -192,6 +192,11 @@ restorecon_private (char const *path, bool local)
     {
       if (getfscreatecon (&tcon) < 0)
         return rc;
+      if (!tcon)
+        {
+          errno = ENODATA;
+          return rc;
+        }
       rc = lsetfilecon (path, tcon);
       freecon (tcon);
       return rc;

Message #14 received at 16335 <at> debbugs.gnu.org (full text, mbox):

From: Nicolas Iooss <nicolas.iooss <at> m4x.org>
To: 16335 <at> debbugs.gnu.org
Subject: Re: bug#16335: Segmentation fault when using cp -a with SELinux and
 fakeroot
Date: Sat, 04 Jan 2014 12:02:14 +0100

On 04/01/2014 04:03, Pádraig Brady wrote :
> On 01/04/2014 01:42 AM, Pádraig Brady wrote:
>> On 01/03/2014 10:08 PM, Nicolas Iooss wrote:
>>> Hello,
>>>
>>> After upgrading to coreutils 8.22 I can no longer build packages which
>>> uses "cp -a" to copy files due to a segmentation fault happening in
>>> libselinux.
>>>
>>> I've tried to reproduce this bug with few commands, in a directory which
>>> doesn't have any default context:
>>>
>>>     $ mkdir /tmp/foobar
>>>     $ matchpathcon
>>>     /tmp/foobar	<<none>>
>>>     $ touch /tmp/foobar/a
>>>     $ fakeroot cp -a /tmp/foobar/a /tmp/foobar/b
>>>     $ fakeroot cp -a /tmp/foobar/a /tmp/foobar/b
>>>     /usr/bin/fakeroot: line 181:  9207 Segmentation fault
>>>
>>> Without fakeroot there is no segmentation fault.
>>>
>>> Even if the message says "/usr/bin/fakeroot", a coredump has been
>>> created for cp. I've analyzed this dump using gdb and after some
>>> debugging, I found out that restorecon_private (from src/selinux.c) was
>>> calling lsetfilecon with a NULL security context which was obtained by
>>> getfscreatecon (case "local = true" in the code [1]). This causes a null
>>> pointer dereference in libselinux and so a SIGSEGV.
>>>
>>> I've reported this bug to libselinux maintainers [2] and got the reply
>>> that calling lsetfilecon with a NULL security context was like calling
>>> strlen with a NULL string and that this was a problem in caller's code [3].
>>>
>>> Hence I propose the attached patch to fix the segmentation fault. Could
>>> you please accept it?
>>>
>>> When you reply, please Cc me as I'm not subscribed.
>>>
>>> Thanks,
>>>
>>> Nicolas Iooss
>>>
>>> -----------
>>>
>>> System configuration during my tests:
>>>
>>> * distro: ArchLinux which SELinux packages
>>> * CPU arch: x86_64
>>> * SELinux in permissive mode
>>> * coreutils 8.22
>>> * libselinux 2.2.1
>>> * fakeroot 1.20
>>>
>>> [1]
>>> http://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=blob;f=src/selinux.c;hb=v8.22#l191
>>> [2] http://marc.info/?l=selinux&m=138763485330568&w=2
>>> [3] http://marc.info/?l=selinux&m=138842015508829&w=2
>>
>> Thanks for the very thorough analysis and patch.
>> The patch looks correct as getfscreatecon() is
>> documented to return a NULL context in some cases.
>> I'll see if I can add a robust test and will apply
>> this in your name.

Thanks for your quick reply.

> 
> Actually what's errno set to with tcon is NULL.
> If if was 0 you might get the classic "error success" message
> if using the --preserve=context option rather than -a for example.

According to libselinux code [4], when "fscreate" attribute is empty,
getfscreatecon sets the security context to NULL and returns 0 without
setting errno. Hence if it remains zero, set_file_security_ctx from
src/copy.c will report the "error success" message.

> 
> I.E. the following might be more appropriate.

I agree. With your patch I get this (as expected):

    $ fakeroot cp --preserve=context a b
    cp: failed to get security context of 'a': No data available

> Note neither Fedora 15 or 20 here produce a NULL value with fakeroot.

On my system, fakeroot (version 1.20) doesn't seem to support xattr:

   $ fakeroot getfattr -m - -d /tmp/foobar/a
   $ getfattr -m - -d /tmp/foobar/a
   getfattr: Suppression des « / » en tête des chemins absolus
   # file: tmp/foobar/a
   security.selinux="unconfined_u:object_r:user_tmp_t:s0"

> 
> thanks,
> Pádraig.
> 
> diff --git a/src/selinux.c b/src/selinux.c
> index cd38a81..016db16 100644
> --- a/src/selinux.c
> +++ b/src/selinux.c
> @@ -192,6 +192,11 @@ restorecon_private (char const *path, bool local)
>      {
>        if (getfscreatecon (&tcon) < 0)
>          return rc;
> +      if (!tcon)
> +        {
> +          errno = ENODATA;
> +          return rc;
> +        }
>        rc = lsetfilecon (path, tcon);
>        freecon (tcon);
>        return rc;
> 

Nicolas

[4]
http://userspace.selinuxproject.org/trac/browser/libselinux/src/procattr.c?rev=edc2e99687b050d5be21a78a66d038aa1fc068d9#L176

Message #19 received at 16335-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Nicolas Iooss <nicolas.iooss <at> m4x.org>
Cc: 16335-done <at> debbugs.gnu.org
Subject: Re: bug#16335: Segmentation fault when using cp -a with SELinux and
 fakeroot
Date: Mon, 13 Jan 2014 14:50:13 +0000

[Message part 1 (text/plain, inline)]

I'm going to push the attached very soon, to address this.

thanks,
Pádraig.

[cp-selinux-segfault.patch (text/x-patch, attachment)]

Message #22 received at 16335 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: 16335 <at> debbugs.gnu.org, nicolas.iooss <at> m4x.org
Subject: Re: bug#16335: Segmentation fault when using cp -a with SELinux and
 fakeroot
Date: Mon, 13 Jan 2014 14:57:20 +0000

On 01/13/2014 02:50 PM, Pádraig Brady wrote:
> +# Then compile/link it:
> +$CC -shared -fPIC -O2 k.c -o k.so \
> +  || framework_failure_ 'failed to build SELinux shared library'

I'll change that to a || skip_ ...
so that we avoid issues with no (stub) <selinux/selinux.h> being available.

thanks,
Pádraig.

Message #25 received at 16335 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Pádraig Brady <P <at> draigBrady.com>, 
 16335 <at> debbugs.gnu.org, nicolas.iooss <at> m4x.org
Subject: Re: bug#16335: Segmentation fault when using cp -a with SELinux and
 fakeroot
Date: Mon, 13 Jan 2014 16:27:40 +0100

On 01/13/2014 03:57 PM, Pádraig Brady wrote:
> On 01/13/2014 02:50 PM, Pádraig Brady wrote:
>> +# Then compile/link it:
>> +$CC -shared -fPIC -O2 k.c -o k.so \
>> +  || framework_failure_ 'failed to build SELinux shared library'
> 
> I'll change that to a || skip_ ...
> so that we avoid issues with no (stub) <selinux/selinux.h> being available.

LD_PRELOADed tests are sometimes a bit tricky, so doing
double checks is a good idea: I'd add a
  fclose(fopen("x"));
inside the dummies, and check if that file has really been
created. Otherwise, you can't be sure if replacing the functions
really worked.

Furthermore, when I added a LD_PRELOADed test a while ago,
I think Paul suggested to add -ldl for some non-GNU/Linux
platforms. I'd also specify 'gcc' hardcoded ... probably
with -Wall.

+1 otherwise.

Thanks & have a nice day,
Berny

Message #28 received at 16335 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>
Cc: nicolas.iooss <at> m4x.org, 16335 <at> debbugs.gnu.org
Subject: Re: bug#16335: Segmentation fault when using cp -a with SELinux and
 fakeroot
Date: Mon, 13 Jan 2014 17:09:54 +0000

On 01/13/2014 03:27 PM, Bernhard Voelker wrote:
> On 01/13/2014 03:57 PM, Pádraig Brady wrote:
>> On 01/13/2014 02:50 PM, Pádraig Brady wrote:
>>> +# Then compile/link it:
>>> +$CC -shared -fPIC -O2 k.c -o k.so \
>>> +  || framework_failure_ 'failed to build SELinux shared library'
>>
>> I'll change that to a || skip_ ...
>> so that we avoid issues with no (stub) <selinux/selinux.h> being available.
> 
> LD_PRELOADed tests are sometimes a bit tricky, so doing
> double checks is a good idea: I'd add a
>   fclose(fopen("x"));
> inside the dummies, and check if that file has really been
> created. Otherwise, you can't be sure if replacing the functions
> really worked.

Right, I'll skip_ in that case to warn
about stale tests.

> Furthermore, when I added a LD_PRELOADed test a while ago,
> I think Paul suggested to add -ldl for some non-GNU/Linux
> platforms.

Right. I'll refactor all those calls to a gcc_shared_() for consistency.

> I'd also specify 'gcc' hardcoded ... probably with -Wall.

Hmm, icc and clang support this gcc interface,
so I'm inclined to leave it as $CC so as not
preclude those from this part of the testing matrix.
We can always beef up require_gcc_shared_() if
this ever becomes an issue.

thanks!
Pádraig.

Message #31 received at 16335 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>
Cc: nicolas.iooss <at> m4x.org, 16335 <at> debbugs.gnu.org
Subject: Re: bug#16335: Segmentation fault when using cp -a with SELinux and
 fakeroot
Date: Mon, 13 Jan 2014 20:09:49 +0000

[Message part 1 (text/plain, inline)]

On 01/13/2014 05:09 PM, Pádraig Brady wrote:
> On 01/13/2014 03:27 PM, Bernhard Voelker wrote:
>> On 01/13/2014 03:57 PM, Pádraig Brady wrote:
>>> On 01/13/2014 02:50 PM, Pádraig Brady wrote:
>>>> +# Then compile/link it:
>>>> +$CC -shared -fPIC -O2 k.c -o k.so \
>>>> +  || framework_failure_ 'failed to build SELinux shared library'
>>>
>>> I'll change that to a || skip_ ...
>>> so that we avoid issues with no (stub) <selinux/selinux.h> being available.
>>
>> LD_PRELOADed tests are sometimes a bit tricky, so doing
>> double checks is a good idea: I'd add a
>>   fclose(fopen("x"));
>> inside the dummies, and check if that file has really been
>> created. Otherwise, you can't be sure if replacing the functions
>> really worked.
> 
> Right, I'll skip_ in that case to warn
> about stale tests.
> 
>> Furthermore, when I added a LD_PRELOADed test a while ago,
>> I think Paul suggested to add -ldl for some non-GNU/Linux
>> platforms.
> 
> Right. I'll refactor all those calls to a gcc_shared_() for consistency.
> 
>> I'd also specify 'gcc' hardcoded ... probably with -Wall.
> 
> Hmm, icc and clang support this gcc interface,
> so I'm inclined to leave it as $CC so as not
> preclude those from this part of the testing matrix.
> We can always beef up require_gcc_shared_() if
> this ever becomes an issue.

Pushing the attached 2 patches in a while.

thanks,
Pádraig.

[cp-selinux-segfault.patch (text/x-patch, attachment)]

Message #34 received at 16335 <at> debbugs.gnu.org (full text, mbox):

From: Nicolas Iooss <nicolas.iooss <at> m4x.org>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 16335 <at> debbugs.gnu.org
Subject: Re: bug#16335: Segmentation fault when using cp -a with SELinux and
 fakeroot
Date: Mon, 13 Jan 2014 21:12:45 +0100

Le 13/01/2014 15:50, Pádraig Brady a écrit :
> I'm going to push the attached very soon, to address this.
> 
> thanks,
> Pádraig.
> 

Thanks for setting me as the author of this patch. Nevertheless my name
(Iooss) spells with and I (like India) and not L (Lima). Could you fix this?

Nicolas

Message #37 received at 16335 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Nicolas Iooss <nicolas.iooss <at> m4x.org>
Cc: 16335 <at> debbugs.gnu.org
Subject: Re: bug#16335: Segmentation fault when using cp -a with SELinux and
 fakeroot
Date: Mon, 13 Jan 2014 23:14:21 +0000

On 01/13/2014 08:12 PM, Nicolas Iooss wrote:
> Le 13/01/2014 15:50, Pádraig Brady a écrit :
>> I'm going to push the attached very soon, to address this.
>>
>> thanks,
>> Pádraig.
>>
> 
> Thanks for setting me as the author of this patch. Nevertheless my name
> (Iooss) spells with and I (like India) and not L (Lima). Could you fix this?

Done and pushed.

http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commit;h=d718331e5

thanks again,
Pádraig.

Message #40 received at 16335 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: nicolas.iooss <at> m4x.org, 16335 <at> debbugs.gnu.org
Subject: Re: bug#16335: Segmentation fault when using cp -a with SELinux and
 fakeroot
Date: Tue, 14 Jan 2014 11:54:24 +0100

On 01/13/2014 09:09 PM, Pádraig Brady wrote:
> Pushing the attached 2 patches in a while.

Hi Padraig,

thanks, the refactoring into gcc_shared_ is a good idea.

But I missed this one:
when selinux is not supported, the new no-ctx.sh test is skipped
with the wrong and misleading "LD_PRELOAD interception failed" diagnostic:

+ gcc -std=gnu99 -Wall -shared --std=gnu99 -fPIC -ldl -O2 k.c -o k.so
+ touch file_src
+ LD_PRELOAD=./k.so
+ cp -a file_src file_dst
+ LD_PRELOAD=./k.so
+ cp -a file_src file_dst
+ LD_PRELOAD=./k.so
+ cp --preserve=context file_src file_dst
cp: cannot preserve security context without an SELinux-enabled kernel
+ test -e preloaded
+ skip_ 'LD_PRELOAD interception failed'
+ warn_ 'no-ctx.sh: skipped test: LD_PRELOAD interception failed'
+ case $IFS in
+ printf '%s\n' 'no-ctx.sh: skipped test: LD_PRELOAD interception failed'
no-ctx.sh: skipped test: LD_PRELOAD interception failed
+ test 9 = 2
+ printf '%s\n' 'no-ctx.sh: skipped test: LD_PRELOAD interception failed'
+ sed 1q
+ Exit 77

I've no time now to analyze further, unfortunately.

Have a nice day,
Berny

Message #43 received at 16335 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>
Cc: nicolas.iooss <at> m4x.org, 16335 <at> debbugs.gnu.org
Subject: Re: bug#16335: Segmentation fault when using cp -a with SELinux and
 fakeroot
Date: Tue, 14 Jan 2014 11:35:13 +0000

On 01/14/2014 10:54 AM, Bernhard Voelker wrote:
> On 01/13/2014 09:09 PM, Pádraig Brady wrote:
>> Pushing the attached 2 patches in a while.
> 
> Hi Padraig,
> 
> thanks, the refactoring into gcc_shared_ is a good idea.
> 
> But I missed this one:
> when selinux is not supported, the new no-ctx.sh test is skipped
> with the wrong and misleading "LD_PRELOAD interception failed" diagnostic:

> + test -e preloaded
> + skip_ 'LD_PRELOAD interception failed'

Oh right. I think this should restrict the test appropriately...

commit 3620df245a2211dc441e019845f98b91333bda77
Author: Pádraig Brady <P <at> draigBrady.com>
Date:   Tue Jan 14 11:30:51 2014 +0000

    tests: restrict a recent SELinux test to SELinux systems

    * tests/cp/no-ctx.sh: Since the test diagnoses whether the
    intercepted lgetfilecon() calls are actually called or not,
    restrict the test to systems where that occurs.
    The test cases are minimal on non SELinux systems and should
    be well covered by other tests.
    Reported-by: Bernhard Voelker

diff --git a/tests/cp/no-ctx.sh b/tests/cp/no-ctx.sh
index 3b5eb82..6851785 100755
--- a/tests/cp/no-ctx.sh
+++ b/tests/cp/no-ctx.sh
@@ -22,6 +22,7 @@
 . "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
 print_ver_ cp
 require_gcc_shared_
+requires_selinux_

 # Replace each getfilecon and lgetfilecon call with a call to these stubs.
 cat > k.c <<'EOF' || framework_failure_

Message #46 received at 16335 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: nicolas.iooss <at> m4x.org, 16335 <at> debbugs.gnu.org
Subject: Re: bug#16335: Segmentation fault when using cp -a with SELinux and
 fakeroot
Date: Tue, 14 Jan 2014 13:38:27 +0100

On 01/14/2014 12:35 PM, Pádraig Brady wrote:
> On 01/14/2014 10:54 AM, Bernhard Voelker wrote:
>> + test -e preloaded
>> + skip_ 'LD_PRELOAD interception failed'
> 
> Oh right. I think this should restrict the test appropriately...
> 
> commit 3620df245a2211dc441e019845f98b91333bda77
> Author: Pádraig Brady <P <at> draigBrady.com>
> Date:   Tue Jan 14 11:30:51 2014 +0000
> 
>     tests: restrict a recent SELinux test to SELinux systems
> 
>     * tests/cp/no-ctx.sh: Since the test diagnoses whether the
>     intercepted lgetfilecon() calls are actually called or not,

The witness file is only created for getfilecon() - not for
lgetfilecon().

>     restrict the test to systems where that occurs.
>     The test cases are minimal on non SELinux systems and should
>     be well covered by other tests.
>     Reported-by: Bernhard Voelker
> 
> diff --git a/tests/cp/no-ctx.sh b/tests/cp/no-ctx.sh
> index 3b5eb82..6851785 100755
> --- a/tests/cp/no-ctx.sh
> +++ b/tests/cp/no-ctx.sh
> @@ -22,6 +22,7 @@
>  . "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
>  print_ver_ cp
>  require_gcc_shared_
> +requires_selinux_
> 
>  # Replace each getfilecon and lgetfilecon call with a call to these stubs.
>  cat > k.c <<'EOF' || framework_failure_

I'm a bit biased about this patch. Okay, it's perfectly valid to
skip the test if the system doesn't support SELinux, but OTOH it may
be quite valuable to verify the exit codes like that on non-SELinux
systems, i.e., based on stderr of the last cp call, the "preloaded"
file must exist or not. The test could verify that. WDYT?

Thanks & have a nice day,
Berny

Message #49 received at 16335 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>
Cc: nicolas.iooss <at> m4x.org, 16335 <at> debbugs.gnu.org
Subject: Re: bug#16335: Segmentation fault when using cp -a with SELinux and
 fakeroot
Date: Tue, 14 Jan 2014 13:55:22 +0000

On 01/14/2014 12:38 PM, Bernhard Voelker wrote:
> On 01/14/2014 12:35 PM, Pádraig Brady wrote:
>> On 01/14/2014 10:54 AM, Bernhard Voelker wrote:
>>> + test -e preloaded
>>> + skip_ 'LD_PRELOAD interception failed'
>>
>> Oh right. I think this should restrict the test appropriately...
>>
>> commit 3620df245a2211dc441e019845f98b91333bda77
>> Author: Pádraig Brady <P <at> draigBrady.com>
>> Date:   Tue Jan 14 11:30:51 2014 +0000
>>
>>     tests: restrict a recent SELinux test to SELinux systems
>>
>>     * tests/cp/no-ctx.sh: Since the test diagnoses whether the
>>     intercepted lgetfilecon() calls are actually called or not,
> 
> The witness file is only created for getfilecon() - not for
> lgetfilecon().

In the wrapper, lgetfilecon() calls getfilecon() ?

>>     restrict the test to systems where that occurs.
>>     The test cases are minimal on non SELinux systems and should
>>     be well covered by other tests.
>>     Reported-by: Bernhard Voelker
>>
>> diff --git a/tests/cp/no-ctx.sh b/tests/cp/no-ctx.sh
>> index 3b5eb82..6851785 100755
>> --- a/tests/cp/no-ctx.sh
>> +++ b/tests/cp/no-ctx.sh
>> @@ -22,6 +22,7 @@
>>  . "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
>>  print_ver_ cp
>>  require_gcc_shared_
>> +requires_selinux_

BTW that should be require_selinux_
It's dangerous that we don't diagnose such typos.
I wonder would it be appropriate to have a test_require_()
wrapper that would catch such things, and be called like:
  test_require_ gcc_shared selinux

>>  # Replace each getfilecon and lgetfilecon call with a call to these stubs.
>>  cat > k.c <<'EOF' || framework_failure_
> 
> I'm a bit biased about this patch. Okay, it's perfectly valid to
> skip the test if the system doesn't support SELinux, but OTOH it may
> be quite valuable to verify the exit codes like that on non-SELinux
> systems,

Well I did state that "The test cases are minimal on non SELinux systems
and should be well covered by other tests"...

> i.e., based on stderr of the last cp call, the "preloaded"
> file must exist or not. The test could verify that. WDYT?

...and if the last cp fails it could be due to the wrapper running,
or SELinux not being supported. We'd need something else to
distinguish here, and require_selinux_ is the best I can think of
at present.

I suppose an alternative would be to refactor require_selinux_
to a function that just determines if it's available and do:

test -e preloaded ||
  { have_selinux_ && framework_failure_ 'LD_PRELOAD interception failed'; }

thanks,
Pádraig.

Message #52 received at 16335 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: nicolas.iooss <at> m4x.org, 16335 <at> debbugs.gnu.org
Subject: Re: bug#16335: Segmentation fault when using cp -a with SELinux and
 fakeroot
Date: Tue, 14 Jan 2014 17:36:14 +0100

On 01/14/2014 02:55 PM, Pádraig Brady wrote:
> On 01/14/2014 12:38 PM, Bernhard Voelker wrote:
>>>     * tests/cp/no-ctx.sh: Since the test diagnoses whether the
>>>     intercepted lgetfilecon() calls are actually called or not,
>>
>> The witness file is only created for getfilecon() - not for
>> lgetfilecon().
> 
> In the wrapper, lgetfilecon() calls getfilecon() ?

Ah, sure. I missed that, sorry.

>>> diff --git a/tests/cp/no-ctx.sh b/tests/cp/no-ctx.sh
>>> index 3b5eb82..6851785 100755
>>> --- a/tests/cp/no-ctx.sh
>>> +++ b/tests/cp/no-ctx.sh
>>> @@ -22,6 +22,7 @@
>>>  . "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
>>>  print_ver_ cp
>>>  require_gcc_shared_
>>> +requires_selinux_
> 
> BTW that should be require_selinux_
> It's dangerous that we don't diagnose such typos.
> I wonder would it be appropriate to have a test_require_()
> wrapper that would catch such things, and be called like:
>   test_require_ gcc_shared selinux

Hmm, but that would imply the same problem - if someone
misspells "test_require_" ... for which we could maybe add
a syntax-check rule.
I'm not sure if it's worth the effort - when adding/changing
a test, we have to look into the .log file anyway.

>>>  # Replace each getfilecon and lgetfilecon call with a call to these stubs.
>>>  cat > k.c <<'EOF' || framework_failure_
>>
>> I'm a bit biased about this patch. Okay, it's perfectly valid to
>> skip the test if the system doesn't support SELinux, but OTOH it may
>> be quite valuable to verify the exit codes like that on non-SELinux
>> systems,
> 
> Well I did state that "The test cases are minimal on non SELinux systems
> and should be well covered by other tests"...

okay, I'm fine with that.

>> i.e., based on stderr of the last cp call, the "preloaded"
>> file must exist or not. The test could verify that. WDYT?
> 
> ...and if the last cp fails it could be due to the wrapper running,
> or SELinux not being supported. We'd need something else to
> distinguish here, and require_selinux_ is the best I can think of
> at present.
> 
> I suppose an alternative would be to refactor require_selinux_
> to a function that just determines if it's available and do:
> 
> test -e preloaded ||
>   { have_selinux_ && framework_failure_ 'LD_PRELOAD interception failed'; }

I think we should keep it as simple as possible, therefore I'd
now favor your initial version of the patch (with the typo corrected).

Thanks & have a nice day,
Berny

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.