[PATCH] Fix bug #10587 - Opening directories on SLES's cifsd and Apple's smbx succeeds.

[Jeremy Allison]

On Thu, May 08, 2014 at 09:58:19PM +0200, Stefan (metze) Metzmacher wrote:
> Am 08.05.2014 21:06, schrieb Jeremy Allison:
> > From the commit message:
> > 
> > ----------------------------------------------------------
> > SLES's cifsd and Apple's smbx do not correctly handle FILE_NON_DIRECTORY_FILE
> > which prevents recursive copies in gvfs from working correctly since GVFS
> > tries to open the directory, expecting ENOTDIR, but it suceeds and appears as a
> > zero byte file.
> > 
> > This fix adds code to the synchronous NTCreateX and NTTrans
> > open code that checks if CreateOptions was requested with
> > FILE_NON_DIRECTORY_FILE set, and if the attributes
> > returned include FILE_ATTRIBUTE_DIRECTORY we synchronously
> > close the file handle just opened, and return NT_STATUS_FILE_IS_A_DIRECTORY
> > to the caller.
> > ----------------------------------------------------------
> > 
> > Please review and push if acceptable.
> 
> I'd prefer to fix this in the callers, just let the caller get the
> attributes.

So that's what the gvfs guys were trying to avoid, as it
means extra code inside all callers to deal with the breakage.

If when you say "let the caller get the attributes" you
mean making an extra round trip call to get the attributes
that means either an extra round trip for every open, because
the cli_ntcreate call doesn't return the attributes
on successful open (even though it has them) or it means
adding another varient of cli_ntcreate to the library
ABI that does return the attributes to the caller.

So with no ABI change this is an extra round trip
on every open to deal with broken servers :-(.

> This is too much magic if the cli_foo() behaves differently as
> cli_foo_send/recv().

Yeah, I had qualms about it too, but it's the simplest
fix I could come up with - not being able to change
the gvfs or other libsmbclient callers.

Or do you mean putting this change inside libsmbclient ?

There is also precedence for this kind of change
based on the fallback code inside cli_open() that
tries ntcreateX first, and then fall back to
openX if the server returns specific error codes:

From cli_open()...

/****************************************************************************
 Synchronous wrapper function that does an NtCreateX open by preference
 and falls back to openX if this fails.
****************************************************************************/
....
        /* Try and cope will all varients of "we don't do this call"
           and fall back to openX. */

        if (NT_STATUS_EQUAL(status,NT_STATUS_NOT_IMPLEMENTED) ||
                        NT_STATUS_EQUAL(status,NT_STATUS_INVALID_INFO_CLASS) ||
                        NT_STATUS_EQUAL(status,NT_STATUS_PROCEDURE_NOT_FOUND) ||
                        NT_STATUS_EQUAL(status,NT_STATUS_INVALID_LEVEL) ||
                        NT_STATUS_EQUAL(status,NT_STATUS_INVALID_PARAMETER) ||
                        NT_STATUS_EQUAL(status,NT_STATUS_INVALID_DEVICE_REQUEST) ||
                        NT_STATUS_EQUAL(status,NT_STATUS_INVALID_DEVICE_STATE) ||
                        NT_STATUS_EQUAL(status,NT_STATUS_CTL_FILE_NOT_SUPPORTED) ||
                        NT_STATUS_EQUAL(status,NT_STATUS_UNSUCCESSFUL)) {
                goto try_openx;
        }

I can re-code this to be hidden inside cli_open() specifically
as this is what libsmbclient uses directly (and it already contains
fallback code) and move it out of the cli_ntcreate and cli_nttrans
calls.

Would that be more acceptable ?

Jeremy.