`rmdir` implementation within smbclient

[Anoop C S]

On Mon, 2018-08-06 at 11:32 -0700, Jeremy Allison wrote:
> On Thu, Aug 02, 2018 at 04:06:16PM +0530, Anoop C S via samba-technical wrote:
> > Hi all,
> > 
> > We have the following bug report which I looked at recently:
> > 
> > https://bugzilla.samba.org/show_bug.cgi?id=13204 - rmdir on non-empty
> > directory fails silently
> > 
> > If fsp->initial_delete_on_close is set during a create/open of a
> > directory we attempt to delete it while closing the fnum based on
> > whether it is empty or not. But as of now it is not set as we have
> > this check(directory emptiness) early while opening the directory with
> > FILE_DELETE_ON_CLOSE set as create option. As a result we never attempt
> > to delete the directory while closing the fnum and rmdir
> > return success as we could close the directory.
> > 
> > Does the protocol say that we don't set FILE_DELETE_ON_CLOSE if the
> > directory is non-empty? If not why are we doing so?
> > 
> > If the current logic is as per protocol how do we move about fixing the
> > bug? Is it OK if we modify `rmdir` implementation to add explicit check
> > for directory emptiness?
> 
> Here's the problem. Windows clients assume if you can set
> FILE_DELETE_ON_CLOSE on a handle it will go away on close,
> and don't look at (or show the user) the error returned
> on close. POSIX won't give an error until you try the
> delete and get an error.
> 
> So we try and decide in advance if we can delete something
> (that's what the code in can_set_delete_on_close() is
> doing so we return the correct error on handle open).

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.

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

Or do let me know if you have an entirely different approach in fixing the issue.

Anoop C S.