[PATCH] pathname cleanups

Jeremy Allison jra at samba.org
Mon May 22 16:32:51 UTC 2017

On Sat, May 20, 2017 at 09:21:10PM +0300, Uri Simchoni wrote:
> On 05/19/2017 06:40 AM, Jeremy Allison via samba-technical wrote:
> > I'm getting ready to move the @GMT snapshot
> > name handling into struct smb_filename to
> > clean up a lot of the shadow_copy2 code.
> > 
> > Here are some prerequisite patches that
> > clean up our handling of the SMB1 FLAGS2
> > header value and remove some unneeded
> > bool parameters to functions.
> > 
> > The changes are already tested by the
> > current pathname tests.
> > 
> > Passes full make test locally.
> > 
> > Please review and push if happy !
> > 
> > Cheers,
> > 
> > 	Jeremy.
> > 
> Patches 1-10, RB+ me - pure refactoring (and a bugfix, thanks).

Thanks !

> About the last step, I just want to make sure I understand what we're
> trying to do, because I got a little confused by the "dumb clients"
> discussion:
> If I understand correctly, "dumb clients" are not
> previous-version-aware, they just want to reach any path that the OS
> exposes below the share's root.
> Before the change, if the access path was
> "somedir/.snapshots/@GMT-foo/file", we would "canonicalize" that into
> "GMT-foo/somedir/.snapshots/file", and then we would not find the file,
> regardless of whether shadow_copy2 is loaded or not.

Not quite. The security fix change canonicalizes the
name into "@GMT-foo/somedir/.snapshots/file" to ensure
that when we have to do:


for security reasons that the @GMT-foo is guaranteed to be
seen by shadow_copy2 (as it starts the path).

Without the canonicalization the @GMT-foo may get lost,
if it was at the end of the path.

> After the change, we do the "canonicalization" only if the client
> indicated that it wants a previous version, hence the dumb-client
> problem is fixed. Samba clients only do that indication starting at
> 4.5.0, but even prior to that, the canonicalization doesn't change
> anything for Samba clients, which put the @GMT tag at the beginning.
> If that's what we want to do - fine, makes sense, RB+ too.

Yep - that's the goal. Thanks !

> However, some things don't add up which puts my understanding in question:
> 1. Why do we need this canonicalization at all then? Is there a
> previous-versions-aware client that puts the @GMT not at the beginning?
> Or we just want to follow the spec to the letter, meaning that the @GMT
> is allowed to be anywhere, and the decisive factor of whether it's a
> previous version request is that flag.

Yes. The SMB2 code has to put the @GMT at the end, as otherwise
we may lose the incoming \\server\share\dfs\absolute\path (this
was an earlier bugfix). That's why my patch adds in the FLAGS2
flag to the fake SMB1 struct in the SMB2-create-with-timestamp
code path.

The canonicalization allows this to work.

> 2. Once this is fixed, why would we need to keep all the "horrible
> complexity" inside vfs_shadow_copy2? Let's assume our next step is to
> remove @GMT into a timestamp field *only if* it's the first component of
> the path, *after* it's been canonicalized, and vfs_shadow_copy2 handles
> only paths with a timestamp (i.e. it's greatly simplified). What would
> not work then?

So most of the "horrible complexity" inside vfs_shadow_copy2 turns out
to be related to where the snapshot paths are on disk, not in the
code that finds the SMB-layer @GMT- string. So yes, once I've fixed
all VFS pathnames to be struct smb_filename I can then remove the
SMB-layer @GMT-foo IF the flag is set and IF the @GMT is at the
start (after canonicalization). This will simplify one of
the parsing code paths inside vfs_shadow_copy2, but the main thing
it'll allow is fast detection of incoming previous version paths.

On another related note - the reason this will fix Volker's "old SMB1 clients
with no flag issue" is that with the patch the incoming client pathname
will be left completely alone and be passed into vfs_shadow_copy2
as-is, restoring the old behavior.

These old clients might run into the chdir()/open() issue if
they put the @GMT at the end of the path, but if that's really
an issue they can turn off symlink restrictions and remove
the chdir()/open() changes if they simply *MUST* make this
work (at the cost of removing the symlink race protection).

> Beside all that, I have to wonder about file renaming - does it really
> make sense to regard the destination file name as a potential DFS path
> if the source file name is a DFS path? (off-topic but I did some digging
> around rename lately, [MS-FSCC] FILE_RENAME_INFORMATION section doesn't
> mention DFS, and I'm not much of a DFS expert...)

Yep, eventually I'm going to have to add more tests here
(although you may already have found this out by your
pinging of dochelp :-).



More information about the samba-technical mailing list