`rmdir` implementation within smbclient

[Anoop C S]

On Thu, 2018-08-09 at 14:04 +0530, Anoop C S via samba-technical wrote:
> On Wed, 2018-08-08 at 16:52 -0700, Jeremy Allison wrote:
> > On Wed, Aug 08, 2018 at 11:42:52AM -0700, Jeremy Allison via samba-technical wrote:
> > > On Thu, Aug 09, 2018 at 12:05:58AM +0530, Anoop C S wrote:
> > > > 
> > > > Also `rmdir` is handled differently for client connections with protocol dialects
> > > > > =PROTOCOL_SMB2_02
> > > > 
> > > > when compared to those below PROTOCOL_SMB2_02 inside cli_rmdir().
> > > > NT_STATUS_DIRECTORY_NOT_EMPTY is
> > > > returned correctly on non-empty directory for client connection below PROTOCOL_SMB2_02 using
> > > > smb_command SMBrmdir(with send, recv approach).
> > > > 
> > > > Whereas for higher protocol clients we go via the procedure of creating a fnum with
> > > > FILE_DELETE_ON_CLOSE and assumes to get it deleted while closing the handle(fnum). But
> > > > inside
> > > > open_directory() we purposefully skip NT_STATUS_DIRECTORY_NOT_EMPTY and fnum creation is
> > > > successful
> > > > without setting fsp->initial_delete_on_close. Thus a non-empty directory is not deleted
> > > > during
> > > > a
> > > > subsequent close unless we explicitly set delete_on_close token i.e, `rmdir` fakes directory
> > > > deletion.
> > > > 
> > > > Therefore I can think of the following ways to fix this:
> > > > [1] Do not skip NT_STATUS_DIRECTORY_NOT_EMPTY for can_set_delete_on_close() inside
> > > > open_directory()
> > > > to return the error.
> > > 
> > > I'm guessing (although I'll need to check Windows to be sure)
> > > that setting FILE_DELETE_ON_CLOSE on a non-empty directory
> > > handle open over SMB2 to Windows doesn't error out. We need
> > > a test for this if we don't already have one.
> 
> I couldn't find a test to validate the above scenario. The only test which creates something
> inside
> a directory is deltest20. But it checks whether NT_STATUS_DELETE_PENDING is returned while trying
> to
> create a file inside a directory on which DELETE_ON_CLOSE is set and whether
> NT_STATUS_DIRECTORY_NOT_EMPTY is returned while trying to set DELETE_ON_CLOSE on a non-empty
> directory.
> 
> I will add a subtest deltest20c for verification.

Attached a sample test.

> > > If Windows errors out in this case, then that's the correct fix.
> > > 
> > > The slightly tricky case here is if "delete veto files = yes"
> > > is set. I need to think about that some more.
> > > 
> > > If not...
> > 
> > Yep, just tested it. As expected, Windows does not error out
> > error out when a non-empty directory is opened with DELETE_ON_CLOSE
> > set, it just allows the open and doesn't set delete on close
> > (the close doesn't delete it, obviously).
> Thanks for confirming the behaviour.
> 
> > > > [2] Make sure that we do either of the following to return NT_STATUS_DIRECTORY_NOT_EMPTY
> > > > during a
> > > > subsequent close:
> > > >     (a) set fsp->initial_delete_on_close by handling NT_STATUS_DIRECTORY_NOT_EMPTY
> > > > separately
> > > > inside
> > > >         open_directory().
> > > >         OR
> > > >     (b) explicitly set delete_on_close token using cli_smb2_delete_on_close() after creating
> > > > fnum
> > > >         inside cli_smb2_rmdir(). Note that cli_smb2_delete_on_close() is not yet consumed in
> > > > code
> > > >         base.
> > > 
> > > Then (b) is the correct fix.
> > > 
> > > > Or do let me know if you have an entirely different approach in fixing the issue.
> > > 
> > > Nope, let's do the tests and work it out.
> > 
> > So we need to use cli_smb2_delete_on_close() into cli_smb2_rmdir().
> 
> I will send a patch shortly.
> 
> > Jeremy.
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-s4-torture-Add-new-test-for-DELETE_ON_CLOSE-on-non-e.patch
Type: text/x-patch
Size: 4358 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20180809/23a13473/0001-s4-torture-Add-new-test-for-DELETE_ON_CLOSE-on-non-e.bin>