Bug 6891 - using windows explorer to change ownership on a folder fails with Bad File Descriptor
using windows explorer to change ownership on a folder fails with Bad File De...
Product: Samba 3.4
Classification: Unclassified
Component: File services
Other Windows XP
: P3 normal
: ---
Assigned To: Jeremy Allison
Samba QA Contact
Depends on:
  Show dependency treegraph
Reported: 2009-11-12 02:01 UTC by Barry Sabsevitz
Modified: 2009-11-20 17:12 UTC (History)
1 user (show)

See Also:

Fix for 3.4.4. (711 bytes, patch)
2009-11-12 15:05 UTC, Jeremy Allison
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Barry Sabsevitz 2009-11-12 02:01:38 UTC
When using windows explorer to change ownership on a folder, you will fail with an error that says Bad File Descriptor.

The bug seems to be in try_chown(). If you get to case(4) and issue the call to open_file_fchmod(), it will return NT_STATUS_OK but the fsp->fh->fd will be -1 because the code path that flows through open_file_fchmod() doesn't assign a file descriptor to a directory that is opened. This causes the SMB_VFS_FCHOWN() to fail. One possible fix that Steve found is to use SMB_VFS_CHOWN for directories.

To reproduce:
0. Use acl_xattr module and have dos filemode = yes in smb.conf file.
1. Create a folder as user 1.
2. Add an ACL to the folder to allow user 2 full access.
3. Login as user 2.
4. Try to chownership on the folder using windows explorer to from user 1 to user 2.
Comment 1 Jeremy Allison 2009-11-12 13:31:02 UTC
Ok, as you say the easy way to fix this is to add an "if (fsp->fh->fd == -1)" then use SMB_VFS_CHOWN on the pathname inside the becmome_root() loop. I'll take a look at this.
Comment 2 Jeremy Allison 2009-11-12 13:35:03 UTC
Actually, thinking about it the right way is to always use SMB_VFS_LCHOWN, not CHOWN - as LCHOWN does not follow symlinks. Yes this will break if someone has set up a symlink in the share (which is transparent to Windows clients) but it's safer because it ensures no symlink races are possible.
Comment 3 Jeremy Allison 2009-11-12 13:36:53 UTC
Hmmmm. Although we already have the race in the :

       /* Case (2) / (3) */
        if (lp_enable_privileges()) {

path above, so adding it again won't make things worse than they already are...

Comment 4 Jeremy Allison 2009-11-12 15:05:45 UTC
Created attachment 4954 [details]
Fix for 3.4.4.

This should fix the problem.
Comment 5 Jeremy Allison 2009-11-12 15:06:46 UTC
Once you confirm the bugfix I'll get it reviewed and re-assigned to Karolin for inclusion in 3.4.4.
Thanks !
Comment 6 Jeremy Allison 2009-11-20 16:35:50 UTC
Ping. Have you been able to test this yet ?
Comment 7 Barry Sabsevitz 2009-11-20 17:12:53 UTC
Hey Jeremy, sorry - again due to an internal schedule issue with our release we used a patch very similar to yours but instead of the LCHOWN we had put CHOWN in, and it works for us. Yours is better so we will take your fix as soon as we can.