`rmdir` implementation within smbclient

[Anoop C S]

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.

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