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

[Surbhi Palande]

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 
"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.