bug#16335: Segmentation fault when using cp -a with SELinux and fakeroot

[Bernhard Voelker]

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