Bug 4188 - Windows Vista RC1 and RC2 can't delete directory on Samba share
Summary: Windows Vista RC1 and RC2 can't delete directory on Samba share
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.0
Classification: Unclassified
Component: File Services (show other bugs)
Version: 3.0.23c
Hardware: x86 Linux
: P3 normal
Target Milestone: none
Assignee: Jeremy Allison
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-10-24 16:49 UTC by Greg Zartman
Modified: 2007-04-07 09:45 UTC (History)
4 users (show)

See Also:


Attachments
smb.conf configuration file (5.15 KB, text/plain)
2006-10-24 16:51 UTC, Greg Zartman
no flags Details
Vista RC2 log file (128.22 KB, text/plain)
2006-10-24 16:52 UTC, Greg Zartman
no flags Details
My smb.conf (9.78 KB, text/plain)
2007-01-05 16:07 UTC, Bahram C.
no flags Details
Prevents setting delete_on_close on non-empty directory (1.91 KB, patch)
2007-02-01 14:00 UTC, Joe Meadows
no flags Details
Patch for 3.0.24. (1.92 KB, patch)
2007-02-08 20:01 UTC, Jeremy Allison
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Greg Zartman 2006-10-24 16:49:41 UTC
Open a samba from a Windows Vista RC1 or RC2 and attempt to delete a
folder.  Windows shows the folder as deleted.  Refresh the share window and the
folder reappears.  No problem deleting a folder from the share using Windows
XP.
Comment 1 Greg Zartman 2006-10-24 16:51:39 UTC
Created attachment 2190 [details]
smb.conf configuration file
Comment 2 Greg Zartman 2006-10-24 16:52:38 UTC
Created attachment 2191 [details]
Vista RC2 log file

This is the log level 10 log file for the delete folder attempt
Comment 3 Joakim Plate 2006-11-28 15:17:30 UTC
http://validator.w3.org/check?uri=www.samba.org

doesn't really like it either..
Comment 4 Bahram C. 2006-12-28 14:40:11 UTC
(In reply to comment #3)
> http://validator.w3.org/check?uri=www.samba.org
> 
> doesn't really like it either..
> 

(In reply to comment #2)
> Created an attachment (id=2191) [edit]
> Vista RC2 log file
> 
> This is the log level 10 log file for the delete folder attempt
> 

I'm experiencing the same problem with samba 3.0.23d, but it only 
fails when the directory is not empty.
Comment 5 Bahram C. 2007-01-03 13:45:37 UTC
Setting "delete veto files" to yes and applying the following patch seems to 
fix the problem.


--- ../.build_orig/samba/source/smbd/reply.c    2007-01-03 13:21:54.000000000 -0500
+++ samba/source/smbd/reply.c   2007-01-03 13:25:26.000000000 -0500
@@ -3950,7 +3950,8 @@ BOOL rmdir_internals(connection_struct *

                if(dir_hnd != NULL) {
                        long dirpos = 0;
-                       while ((dname = ReadDirName(dir_hnd,&dirpos))) {
+                       if (!lp_recursive_veto_delete(SNUM(conn))) {
+                          while ((dname = ReadDirName(dir_hnd,&dirpos))) {
                                if((strcmp(dname, ".") == 0) || (strcmp(dname, "..")==0))
                                        continue;
                                if (!is_visible_file(conn, directory, dname, &st, False))
@@ -3959,6 +3960,7 @@ BOOL rmdir_internals(connection_struct *
                                        all_veto_files = False;
                                        break;
                                }
+                          }
                        }
Comment 6 Joe Meadows 2007-01-04 20:22:48 UTC
In my test case I found that Vista is setting 'close on delete' on the directory.  When the directory handle is closed reply_close() calls rmdir_internals() which fails because the directory is not empty.  The solution presented above has a problem, if I set 'delete veto files=yes' then I can go into a DOS box and do 'rmdir some_dir', the directory and all of its contents are deleted instead of correctly giving me a 'directory not empty' error.

It seems that the Vista client is expecting an implicit recursive delete when the directory is closed.  Would it be correct to change reply_close() to do a recursive delete (rather than calling rmdir_internals()) in this case ?
Comment 7 Joe Meadows 2007-01-04 20:26:35 UTC
Pardon, I meant 'delete on close', not 'close on delete' :)
Comment 8 Jeremy Allison 2007-01-05 11:20:36 UTC
I'd like to reproduce this as the patch doesn't look correct to me.
When you say "attempt to delete a folder" I need more info than that.

What *exactly* is in the folder ? Is this just a new folder you've created from scratch ?

Jeremy.
Comment 9 Bahram C. 2007-01-05 16:03:36 UTC
Thanks Jeremy for looking into this issue.

On a Vista workstation, I created a new folder (myfolder), then I created a subfolder under that (mysubfolder). Then I tried to delete the myfolder. Windows show that it is delete, but after refreshing the screen, myfolder comes back. And it is not actually deleted on Linux side too.

Note that this only happens when Samba and Windows are configured as WORKGROUP. When Samba is configured as Domain Controller or Domain Member modes, there is no such problem. I will attach my smb.conf for your reference.
Comment 10 Bahram C. 2007-01-05 16:07:10 UTC
Created attachment 2250 [details]
My smb.conf
Comment 11 Bahram C. 2007-01-05 16:28:51 UTC
By the way, I agree that my patch is hacky, but that was the quickest way for me to workaround it for now.
Comment 12 Lon Feldman 2007-01-05 18:47:40 UTC
From my testing I have that I can easily reproduce this if the folder was configured to be a remote offline sync folder.  The sync offline folders option seems to be different in Vista than previous versions of windows and previous versions of windows do not exhibit this behaviour.  So it seems that samba might not handle these offline folder operations correctly.  
Comment 13 Bahram C. 2007-01-11 12:26:42 UTC
I don't see this problem anymore after upgrading to Windows Vista Business Release.
Comment 14 Greg Zartman 2007-01-11 12:35:15 UTC
I have found this issue to be very intermittent in nature.  It was quite reproducible for me for some time, but now I can't reproduce it.  I am still running release candidate 2, the same version of Samba that I was when I reported this, but don’t see the problem at all.  IMO, we’ll need to wait and see what happens once Vista starts being used by the masses to really know if this is a bug or just a Vista hiccup.

Greg
Comment 15 Lon Feldman 2007-01-11 17:05:15 UTC
(In reply to comment #14)
> I have found this issue to be very intermittent in nature.  It was quite
> reproducible for me for some time, but now I can't reproduce it.  I am still
> running release candidate 2, the same version of Samba that I was when I
> reported this, but don’t see the problem at all.  IMO, we’ll need to wait
> and see what happens once Vista starts being used by the masses to really know
> if this is a bug or just a Vista hiccup.
> 
> Greg
> 

My testing, as of late, is against the production version (MSDN) of the Ultimate Version and I have not seen this unless I am configured as Sync Offline Folder.  

Jeremy, should I open a new bug that is explicit that samba properly does not support the Sync Offline Folder option, or utilize this current bug?
Comment 16 Joe Meadows 2007-02-01 13:59:54 UTC
This problem is consistently reproducible, as Lon discovered, by setting a folder to be 'available offline'.  Even if I have two folders in the same directory, folder A and folder B, and set folder A to 'offline' the problem shows up when I try to delete folder B (all done using Windows Explorer from a Vista client).  I have a possible solution that so far looks promising, described below along with my understanding of the problem.

Failure mode:
When this problem occurs it is because the Vista client sets delete_on_close on the root of the tree to be deleted.  If the directory is *not empty* a real windows client will return an error NT_STATUS_DIRECTORY_NOT_EMPTY but Samba simply sets the delete_on_close flag and returns NT_STATUS_OK which apparently leaves the client thinking that it is done once it closes the directory.  When the directory is closed Samba detects that it is not empty and doesn't delete the directory.

My patch forces an additional check in can_set_delete_on_close to verify that the directory is empty before setting the delete_on_close flag.  If the directory is found to be not empty then NT_STATUS_DIRECTORY_NOT_EMPTY is returned.  With this change in place I am cannot reproduce the problem (was consistent before) and the network capture shows NT_STATUS_DIRECTORY_NOT_EMPTY being returned after which the Vista client proceeds to do the delete recursively.

The patch (attached, against release 3.0.23d):
The part of the code that checks for an empty directory is essentially lifted from the beginning of rmdir_internal() into a function called deleteDirOk() which is called from can_set_delete_on_close().  

There is a build problem when using this patch as-is, swat will not link b/c it includes locking/locking.c (with part of my change) but not smbd/dir.c which has the functions ReadDirName, OpenDir and CloseDir.  I think this can be easily worked out if this proof of concept fix turns out to be viable.  Although swat fails to link smbd does link and I just copied the smbd binary into place for testing.

Comment 17 Joe Meadows 2007-02-01 14:00:56 UTC
Created attachment 2263 [details]
Prevents setting delete_on_close on non-empty directory
Comment 18 Gerald (Jerry) Carter (dead mail address) 2007-02-08 16:15:22 UTC
Jeremy, I'm assigning this one to you.
Comment 19 Jeremy Allison 2007-02-08 16:39:11 UTC
I think this is already fixed in the SAMBA_3_0_25 and SAMBA_3_0 svn trees as I've added the following code into can_set_delete_on_close(). I'll look into porting this to 3.0.24.

Jeremy.

        /* Don't allow delete on close for non-empty directories. */
        if (fsp->is_directory) {
                long offset = 0;
                NTSTATUS status;
                SMB_STRUCT_STAT st;
                struct dptr_struct *dirptr;
                const char *name;

                status = dptr_create(fsp->conn,
                                        fsp->fsp_name,
                                        False,
                                        True,
                                        0,
                                        "*",
                                        True,
                                        0,
                                        &dirptr);
                if (!NT_STATUS_IS_OK(status)) {
                        return status;
                }

                /* Read 3 entries. Ignore first 2 (they're . and .. ) */
                name = dptr_ReadDirName(dirptr, &offset, &st);
                if (!name) {
                        dptr_CloseDir(dirptr);
                        return NT_STATUS_ACCESS_DENIED;
                }
                name = dptr_ReadDirName(dirptr, &offset, &st);
                if (!name) {
                        dptr_CloseDir(dirptr);
                        return NT_STATUS_ACCESS_DENIED;
                }
                name = dptr_ReadDirName(dirptr, &offset, &st);
                dptr_CloseDir(dirptr);
                if (name) {
                        DEBUG(10,("can_set_delete_on_close: got name %s - can't delete\n", name ));
                        return NT_STATUS_DIRECTORY_NOT_EMPTY;
                }
        }
Comment 20 Jeremy Allison 2007-02-08 20:01:07 UTC
Created attachment 2280 [details]
Patch for 3.0.24.

Here's the patch for 3.0.24. Thanks for the work Joe, it was nicer than mine :-).
Comment 21 Gerald (Jerry) Carter (dead mail address) 2007-02-09 07:17:22 UTC
Closed as fixed.
Comment 22 Gerald (Jerry) Carter (dead mail address) 2007-04-07 09:45:31 UTC
actually closing.