Bug 6173 - Cannot rename files with read-only attribute (Access denied)
Summary: Cannot rename files with read-only attribute (Access denied)
Status: RESOLVED WONTFIX
Alias: None
Product: Samba 3.0
Classification: Unclassified
Component: File Services (show other bugs)
Version: 3.0.32
Hardware: x86 Windows XP
: P3 normal
Target Milestone: none
Assignee: Samba Bugzilla Account
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-06 21:24 UTC by Eric Shubert
Modified: 2009-08-17 13:33 UTC (History)
0 users

See Also:


Attachments
configuration file (10.28 KB, application/octet-stream)
2009-03-07 08:50 UTC, Eric Shubert
no flags Details
Allow renaming of readonly files (604 bytes, patch)
2009-08-15 18:38 UTC, Eric Shubert
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Shubert 2009-03-06 21:24:08 UTC
+++ This bug was initially created as a clone of Bug #2813 +++

With Samba samba-3.0.28-1.el5_2.1, an "access denied" error is returned if from the CLI I try to rename a file that has the read-only attribute set (i.e. no owner "w" permission). It will succeed after a warning using the GUI (Explorer).

This behavior is a regression from previous versions of Samba (e.g. 3.0.10) and
inconsistent with Windows (which allows read-only files to be renamed).

To reproduce:

1. On Windows (XP), map N: to a Samba samba-3.0.28 share.

2. Open up Command Prompt and execute the following commands:

N:\>echo test > testfile

N:\>attrib +r testfile

N:\>attrib testfile
A    R     N:\testfile

N:\>ren testfile testren
Access is denied.


Notice that the "ren" command fails with an "Access is denied" error.

I discovered this error when trying to use rsync/cygwin on XP, syncing a local folder with a samba share. rsync fails on every read-only file, at the end of copying the file when it's attempting to rename the target from the temporary hidden name to the real name.

See also bug report 3035, which allegedly fixed this problem.
Comment 1 Jeremy Allison 2009-03-06 22:55:23 UTC
3.0.28 is very old. The latest Samba 3.0.x release is 3.0.24. Please try and reproduce with this version before reporting this bug (I believe this is a bug we have already fixed).
Jeremy.
Comment 2 Volker Lendecke 2009-03-06 23:26:03 UTC
3.0.34, not 3.0.24 certainly :-)
Comment 3 Eric Shubert 2009-03-07 08:50:23 UTC
Created attachment 3983 [details]
configuration file

Filesystem is mounted with user_xattr in /etc/fstab:
/dev/VolGroup00/LogVol00 /          ext3    defaults,user_xattr  1 1
Comment 4 Eric Shubert 2009-03-07 08:53:08 UTC
I was under the impression as well that this bug was fixed, and quite some time ago as indicated by the bug history.

I've upgraded to 3.0.32, and the symptoms persist.

Perhaps my configuration is errant. I've attached it for your perusal.

What next?
Comment 5 Eric Shubert 2009-03-19 13:02:00 UTC
In the source version 3.0.28, this is what I'm seeing in reply.c:
<paste>
        /* Fix for bug #3035 from SATOH Fumiyasu <fumiyas@miraclelinux.com>

          On a Windows share, a file with read-only dosmode can be opened with
          DELETE_ACCESS. But on a Samba share (delete readonly = no), it
          fails with NT_STATUS_CANNOT_DELETE error.

          This semantic causes a problem that a user can not
          rename a file with read-only dosmode on a Samba share
          from a Windows command prompt (i.e. cmd.exe, but can rename
          from Windows Explorer).
        */

        if (!lp_delete_readonly(SNUM(conn))) {
                if (fattr & aRONLY) {
                        return NT_STATUS_CANNOT_DELETE;
                }
        }
</paste>

Here's the patch from Bug 3035:
<patch>
Index: SAMBA_3_0/source/smbd/reply.c
===================================================================
--- SAMBA_3_0/source/smbd/reply.c	(revision 12955)
+++ SAMBA_3_0/source/smbd/reply.c	(working copy)
@@ -1888,7 +1888,7 @@ NTSTATUS can_delete(connection_struct *c
 		return NT_STATUS_OBJECT_NAME_INVALID;
 #endif /* JRATEST */
 
-	if (!lp_delete_readonly(SNUM(conn))) {
+	if (!check_is_at_open && !lp_delete_readonly(SNUM(conn))) {
 		if (fattr & aRONLY) {
 			return NT_STATUS_CANNOT_DELETE;
 		}
</patch>

Looks to me like the source was well commented, but the functional part of the patch is not there. Am I missing something?

I'll rebuild my 3.0.28 version with this patch, test the result and report back.
Comment 6 Eric Shubert 2009-03-19 14:30:14 UTC
Just a short update. When I try building with the patch from bug 3035 using v3.0.28 (source from EL5 package), I get:
smbd/reply.c: In function ‘can_delete’:
smbd/reply.c:1919: error: ‘check_is_at_open’ undeclared (first use in this function)

I'll investigate further as I have time.
Comment 7 Eric Shubert 2009-03-20 11:09:32 UTC
Another quick update.

.) I've checked the current (3.2.4) source and find no evidence of the patch other than the comments in smbd/reply.c.

.) "check_is_at_open" appears to me to be pseudo-code.

.) As a temporary workaround, I changed my configuration to specify "delete readonly = yes" which would bypass this particular part of the code entirely. I was a bit surprised to see the error persist. This indicates to me that this is not at all the appropriate place for a fix.

I'll look through the source to see if I can determine where the fix should really go.

Oh, and one more thing. I tested renaming of a read-only file on Vista from Explorer(GUI), and it doesn't even give a warning (as XP does), it just does it. Appears to me as though renaming a read-only file should simply be always allowed.
Comment 8 Eric Shubert 2009-03-20 16:28:36 UTC
The code that is triggering the Access Denied condition appears to be in nttrans.c beginning at line 687 (v3.0.28):
        /* Setting FILE_SHARE_DELETE is the hint. */

        if (lp_acl_check_permissions(SNUM(conn))
            && (create_disposition != FILE_CREATE)
            && (share_access & FILE_SHARE_DELETE)
            && (access_mask & DELETE_ACCESS)) {
                if ((dos_mode(conn, fname, &sbuf) & FILE_ATTRIBUTE_READONLY) ||
                                !can_delete_file_in_directory(conn, fname)) {
                        restore_case_semantics(conn, file_attributes);
                        END_PROFILE(SMBntcreateX);
                        return ERROR_NT(NT_STATUS_ACCESS_DENIED);
                }
        }

I've no idea (yet) what that comment means.

Now to figure out what the proper fix should be. Should the readonly test simply be removed, or are there other conditions when it's (still) needed? When we're renaming the file we shouldn't care if readonly is set or not.

Is someone intimately familiar with the context here? I get the feeling I'm on my own here.
Comment 9 Eric Shubert 2009-04-17 16:23:53 UTC
I modified the source, replacing:
                if ((dos_mode(conn, fname, &sbuf) & FILE_ATTRIBUTE_READONLY) ||
                                !can_delete_file_in_directory(conn, fname)) {
with:
                if (!can_delete_file_in_directory(conn, fname)) {
Tested the change, and it no longer fails. This is indeed the code which triggers the failure.

I'm not suggesting that this is a proper fix. I've no idea what other situations are occurring at this point in the code (if any). I do know that if/when a file is being renamed, then it should not be failing due to FILE_ATTRIBUTE_READONLY being set.

Comment 10 Eric Shubert 2009-08-15 18:38:50 UTC
Created attachment 4565 [details]
Allow renaming of readonly files

Here's a patch that I applied to 3.0.33. While it resolves the problem, I'm not absolutely certain that it does not allow something it shouldn't (ie breaks something else).
Comment 11 Jeremy Allison 2009-08-17 13:33:00 UTC
3.0.x is now out of maintenance. If you can reproduce this problem with 3.4.x please re-open the bug.
Jeremy.