Bug 488674

Summary: getfacl/setfacl --physical mode does not work
Product: [Fedora] Fedora Reporter: Pavel Zhukov <pavel>
Component: aclAssignee: Kamil Dudka <kdudka>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: 11CC: gnugv_maintainer, jmoskovc, leo, pavel, steved
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: 2.2.49-2.fc11 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-01-07 21:42:28 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On: 541405    
Bug Blocks: 550346    
Attachments:
Description Flags
/etc/fstab
none
This patch should fix the bug. Note: It's not from upstream.
none
patch for setfacl.c
none
Patch improved again kdudka: review+

Description Pavel Zhukov 2009-03-05 06:20:25 UTC
Created attachment 334099 [details]
/etc/fstab

Description of problem:
physical mode does not work (follow symlink)


Version-Release number of selected component (if applicable):
rpm -q acl
acl-2.2.47-3.fc10.i386

How reproducible:
I has tried to change acl of /home/user directory to access from other OS (from Fedora  to RH /home). But /home/user dir contains symlink to CIFS filesystems (samba). I has tried setfacl -RP -m u:user:r /mnt/redhat/home/user but get messages that setfacl tries to change acl of CIFS content. After that I boot RedHat and try to change acl of Fedora's /home - it work properly (no follow symlink to CIFS)


Steps to Reproduce:
1. change /etc/fstab to acl options
2. mount -o remount /mnt/redhat/
3. setfacl  -RP -m u:pavel:r /mnt/redhat/home/user/
setfacl: /mnt/redhat/home/user//work/mydocs/..... : Operation not supported


  
Actual results:


Expected results:


Additional info:

ll /mnt/redhat/home/user/ | grep work
lrwxrwxrwx   1 root root        6 Mar  5 08:55 work -> /work/

Comment 1 Bug Zapper 2009-11-18 11:17:08 UTC
This message is a reminder that Fedora 10 is nearing its end of life.
Approximately 30 (thirty) days from now Fedora will stop maintaining
and issuing updates for Fedora 10.  It is Fedora's policy to close all
bug reports from releases that are no longer maintained.  At that time
this bug will be closed as WONTFIX if it remains open with a Fedora 
'version' of '10'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version' 
to a later Fedora version prior to Fedora 10's end of life.

Bug Reporter: Thank you for reporting this issue and we are sorry that 
we may not be able to fix it before Fedora 10 is end of life.  If you 
would still like to see this bug fixed and are able to reproduce it 
against a later version of Fedora please change the 'version' of this 
bug to the applicable version.  If you are unable to change the version, 
please add a comment here and someone will do it for you.

Although we aim to fix as many bugs as possible during every release's 
lifetime, sometimes those efforts are overtaken by events.  Often a 
more recent Fedora release includes newer upstream software that fixes 
bugs or makes them obsolete.

The process we are following is described here: 
http://fedoraproject.org/wiki/BugZappers/HouseKeeping

Comment 2 Kamil Dudka 2009-12-17 12:41:58 UTC
Thank you for the report! I can confirm the bug.

It will be fixed in Fedora 11 along with the same bug in getfacl.

Comment 3 Kamil Dudka 2009-12-17 13:41:41 UTC
Both of them seem to be fixed in the new upstream bugfix release acl-2.2.48.

Comment 4 Kamil Dudka 2009-12-17 13:51:24 UTC
*** Bug 488568 has been marked as a duplicate of this bug. ***

Comment 5 M. Steinborn 2009-12-18 15:15:38 UTC
I cannot confirm that this bug is fixed in version 2.2.49, see the upstream bug report http://savannah.nongnu.org/bugs/?28131 for my testcase.

Comment 6 Kamil Dudka 2009-12-20 11:44:04 UTC
Weird, I've just tested your example from the upstream tracker and it works for me fine with 2.2.49. Please test the new packages and set their karma at Bodhi accordingly. If it still fails for you, we'll need some additional info to track it down. Thanks in advance!

Comment 7 Fedora Update System 2009-12-22 04:48:35 UTC
acl-2.2.49-1.fc12 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update acl'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F12/FEDORA-2009-13470

Comment 8 Fedora Update System 2009-12-22 04:58:28 UTC
acl-2.2.49-1.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update acl'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-13523

Comment 9 M. Steinborn 2009-12-26 09:46:00 UTC
Well, after your answer I took a deeper look at the source code. Assume setfacl is called as in my upstream bug report.

After processing the options, the function walk_tree_rec (file: libmisc/walk_tree.c) is called with path ".". A little time later, the same function is called with path "2", which is a symlink.

In line 62 of libmisc/walk_tree.c, it is decided not to follow symlinks (that is correct!). In line 77, lstat(...) is called on the symlink "2".

The conditions in line 79 and line 81 are true, so on line 84 stat(...) is called on the symlink "2". Note that "2" is a dangling symlink in my example (otherwise "ls" would have listed a file named "1"), so stat() failes, because stat follows symlinks in contrast to lstat(). This failure is returned to the caller on line 85.


After this anlysis I believe you missed some point in replying my example. Probably your symlink was not dangling.

Comment 10 M. Steinborn 2009-12-26 09:50:15 UTC
Created attachment 380412 [details]
This patch should fix the bug. Note: It's not from upstream.

A review of the patch would be fine.

Comment 11 Kamil Dudka 2009-12-26 10:24:19 UTC
Created attachment 380416 [details]
patch for setfacl.c

Good catch! It's indeed broken. I should have realized the symlink from example was a dangling symlink. Thanks for being patient!

As for the patch, I don't think the fix should go into walk_tree.c since the same works well for getfacl. Note the walk_tree.c code is also shared with getfattr/setfattr utilities. I simply don't want to introduce any chance in behavior unintentionally.

What about the attached one? I've just taken the working flags for -P from getfacl.

Comment 12 M. Steinborn 2009-12-26 13:16:50 UTC
Created attachment 380426 [details]
Patch improved again 

Well,

you are right about the way to fix this bug.

Fixing the bug in the same way as

http://git.savannah.gnu.org/cgit/acl.git/commit/?id=63451a0

but this time for setfacl seems to be correct. Traversing through tree should be the same for getfacl and setfacl, shouldn't it?


I submitted this patch on the upstream bug report, too. So it is not necessary for forwarding it.

But the position of the first patch should be checked by upstream.



BTW: I am missing mails from bugzilla.redhat.com. But maybe they may take a little longer.

Comment 13 Kamil Dudka 2009-12-26 20:15:04 UTC
Comment on attachment 380426 [details]
Patch improved again 

(In reply to comment #12)
> Fixing the bug in the same way as
> 
> http://git.savannah.gnu.org/cgit/acl.git/commit/?id=63451a0

Thanks for digging it up! It makes sense to me.

> I submitted this patch on the upstream bug report, too. So it is not necessary
> for forwarding it.

Great!

>--- acl-2.2.49.orig/setfacl/setfacl.c	2009-06-29 21:17:25.000000000 +0200
>+++ acl-2.2.49/setfacl/setfacl.c	2009-12-26 13:16:29.951646872 +0100
>@@ -580,13 +580,15 @@
> 				break;
> 
> 			case 'L':  /* follow symlinks */
>-				walk_flags |= WALK_TREE_LOGICAL;
>+				walk_flags |= WALK_TREE_LOGICAL | WALK_TREE_DEREFERENCE;
> 				walk_flags &= ~WALK_TREE_PHYSICAL;
> 				break;
> 
> 			case 'P':  /* do not follow symlinks */
> 				walk_flags |= WALK_TREE_PHYSICAL;
>-				walk_flags &= ~WALK_TREE_LOGICAL;
>+				walk_flags |= WALK_TREE_PHYSICAL;

r+ from me, one minor nit: This line ^^^ is duplicated for no reason, so that I've just removed it.

Thanks for contributing the patch!

Comment 14 Fedora Update System 2009-12-27 20:24:09 UTC
acl-2.2.49-2.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update acl'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-13523

Comment 15 Fedora Update System 2010-01-07 21:42:17 UTC
acl-2.2.49-2.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 16 Fedora Update System 2010-01-07 21:53:40 UTC
acl-2.2.49-2.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.