[PATCH] Prevent the overwriting of global errno in set file info code path.

[Surbhi Palande]

On Wed, 2010-03-17 at 10:34 +0200, Surbhi Palande wrote:
> Hi,
> 
> On Tue, 2010-03-16 at 20:20 +0100, Volker Lendecke wrote:
> > On Tue, Mar 16, 2010 at 10:10:07AM +0200, Surbhi Palande wrote:
> > > BugLink: http://launchpad.net/bugs/276472
> > > 
> > > In the code path for setting the file info, the set_unix_posix_default_acl()
> > > calls set_unix_posix_default_acl() for setting the posix acl. The mapping of
> > > the unix errors to NT errors is based on the global error numbers set by the
> > > set_unix_posix_default_acl(). However this error number is overwritten by
> > > a call to SMB_VFS_SYS_ACL_FREE_ACL() before returning from
> > > set_unix_posix_default_acl(). The overwritten value is a -EINVAL. This
> > > translates to the NT_STATUS_ACCESS_DENIED instead of the expected
> > > NT_STATUS_NOT_SUPPORTED error code.
> > 
> > While the patch looks correct and does not hurt, I would
> > like to know a bit more why it is necessary. Why does
> > acl_free set errno at all? Looking at the manpage I get the
> > impression that it should only set errno if it returns -1.
> 
> Thats correct. I did a saving of the errno in the
> posixacl_sys_acl_set_file() as a safety check, since our intention is to
> have the errno from the acl_set_file(). Normally speaking this should
> not get overwritten at this point. The actual overwriting of errno is
> done by the call to SMB_VFS_SYS_ACL_FREE_ACL() (called from
> set_unix_posix_default_acl)
> 
> > Why in this routine would it set EINVAL? It seems to point
> > at a different bug if acl_free does not like the acl we give
> > it.
> 
> The -EINVAL setting/overwriting takes place by the call to
> SMB_VFS_SYS_ACL_FREE_ACL() (which does a free of an acl entry which is
> already freed by acl_free() called previously). 
> 
> > Before pushing it, can you explain what's going on in your
> > case?
> 
> This is something that happens every single time when you do a cp -p
> file1 file2. cp -p calls a fgetxattr() with a 

The correct call is fsetxattr(), which gets called after fgetxattr()
Sorry for the typo! So fsetxattr() should be returning a "Operation not
permitted" error but it actually returns a "Permission denied" error.  
This happens on a cp -p. Hope that clears things.

> "system.posix_acl_access" which should be returning to the client an
> "Operation not supported" on an underneath extended fs (on the server
> side). But instead the error returned is a "Permission denied" to the
> client side. 
> > 
> > Thanks,
> > 
> > Volker
> 
> Ideally speaking, should not posixacl_sys_acl_set_file() be returning
> the errno (or or errno) that its interested in? However doing so,
> requires changes at quiet a few places and thats why I took the lesser
> affecting path in this patch.
> 
> Thanks!
> 
> Warm Regards,
> Surbhi.
>