bug#25378: cp does not preserve SElinx context of sub folder

[Pádraig Brady]

On 20/02/17 23:18, Bernhard Voelker wrote:
> Hi Padraig,
> 
> only some minor remarks from my side.
> 
> On 02/21/2017 04:11 AM, Pádraig Brady wrote:
>> From d17ca013f3aadcf697663830fa9ec34cba551f66 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?P=C3=A1draig=20Brady?= <address@hidden>
>> Date: Mon, 20 Feb 2017 18:46:49 -0800
>> Subject: [PATCH] cp: set SElinux context for --parents directories
>>
>> * src/copy.h (set_process_security_ctx, set_file_security_ctx):
> _____________^
> 
> minor typo: s/c/h/
> 
>> diff --git a/src/cp.c b/src/cp.c
>> index 88db3a3..6e18263 100644
>> --- a/src/cp.c
>> +++ b/src/cp.c
>> @@ -394,6 +394,8 @@ make_dir_parents_private (char const *const_dir, size_t 
>> src_offset,
>>  
>>    *attr_list = NULL;
>>  
>> +  /* XXX: If all dirs present at the destination,
> ________^^^_____________^
> 
> why "XXX"?  Missing verb "are".

Well I was going to follow up, but might as well here.
It was a bit surprising to me that the presence of all destination
directories would cause no attributes to be copied from the source.
Now it's not particular to SELinux, and also some attributes aren't
copied from the source at all in this code path, so if we were to
address it, it would be a separate patch. I.E. comment added for now.

>> diff --git a/tests/cp/cp-a-selinux.sh b/tests/cp/cp-a-selinux.sh
>> index 19a9e64..de07406 100755
>> --- a/tests/cp/cp-a-selinux.sh
>> +++ b/tests/cp/cp-a-selinux.sh
>> @@ -48,7 +48,6 @@ rm -f f
>>  # due to recursion, and was handled incorrectly in coreutils-8.22
>>  # Note standard permissions are updated for existing directories
>>  # in the destination, so SELinux contexts should be updated too.
>> -chmod o+rw restore/existing_dir
> 
> What was that change for?
> (I don't have a SELinux system here, so the test is skipped here.)

It looked like an unchecked vestige from a separate test script.
So I just removed it since in the area. I'll double check.

> 
>>  mkdir -p backup/existing_dir/ || framework_failure_
>>  ls -Zd backup/existing_dir > ed_ctx || fail=1
>>  grep $ctx ed_ctx && framework_failure_
>> @@ -57,11 +56,31 @@ chcon $ctx backup/existing_dir/file || framework_failure_
>>  # Set the dir context to ensure it is reset
>>  mkdir -p --context="$ctx" restore/existing_dir || framework_failure_
>>  # Copy and ensure existing directories updated
>> -cp -a backup/. restore/
>> +cp -a backup/. restore/ || fail=1
> 
> nice catch, thanks!
> 
>>  ls -Zd restore/existing_dir > ed_ctx || fail=1
>>  grep $ctx ed_ctx &&
>>    { ls -lZd restore/existing_dir; fail=1; }
>>  
>> +# Check context preserved with directories created with --parents,
>> +# which was not handled before coreutils-8.27
> 
> Do we already know the next release will be "8.27"?
> Maybe it's better to change to "<= coreutils-8.26".

Well I was going to announce that I hope to release 8.27
in a couple of weeks, so consider that announced :)

thanks for the review,
Pádraig.