[PATCH] Fix failure to update dirpath

[Anoop C S]

On Mon, 2018-08-20 at 15:29 -0700, Jeremy Allison wrote:
> On Mon, Aug 20, 2018 at 05:17:54PM +0530, Anoop C S via samba-technical wrote:
> > On Sat, 2018-08-18 at 17:02 +0530, Anoop C S via samba-technical wrote:
> > > Hi,
> > > 
> > > Please find the attached patch which updates dirpath where it failed to do so in
> > > check_parent_exists().
> > 
> > Attached is a modified version with a change to where goto label was placed in the previous
> > version
> > of the patch so as to update "start" component too. This should be it.
> > 
> > > Tested with GlusterFS as backend file system.
> > > 
> > > Reviews are appreciated.
> > 
> > Ok.. so a quick re-look at the patch made me think it being too generic to update "start" and
> > "dirpath" in 3 cases rather than specific to the case where last_component and smb_fname are
> > equal. 
> > 
> > Hmm.. actually it should not cause any harm to further processing down the line. Just noting it
> > down
> > here to help whoever wants to review the change.
> 
> OK Anoop I'm reviewing this, and I'd like to understand
> exactly what is the bug you're fixing here.

# smbclient \\\\<IP>\\<share> -U user%passwd
Try "help" to get a list of possible commands.
smb: \>
mkdir testdir
NT_STATUS_INVALID_PARAMETER making remote directory \testdir

> Can you explain what passed in pathnames are not being
> correctly updated ?

So this 'dir_path' allocated string inside unix_convert() is passed onto get_real_filename() call
further down as an empty string("") where it is expected to have a "." if we are creating a new/file
directory inside root of the share being at the root. check_parent_exists() is the place where
'dir_path' is supposed to get updated. 

Implementation of get_real_filename() inside vfs_glusterfs fails while validating 'dir_path' during
a getxattr call and an error(EINVAL) is returned. Whereas in vfs_default it seems to return ENOTSUP
and is skipped to continue with a full directory scan.

The how did it work till now? Ok. Before [1] got in recently GlusterFS used to resolve to root of
the share if path does not start with '/' and we have a valid current working directory.

I am happy to provide more details if required in case its not clear.

[1] http://git.gluster.org/cgit/glusterfs.git/commit/?id=febee007bb1a99d65300630c2a98cbb642b1c8dc

> As far as I can see this is an optimization path which
> isn't required for correctness, so what am I missing ?
> Thanks,
> 
> 	Jeremy.
> 
> > From 06e95bfcc0f61cc32f191bb71bd8f593aad458c6 Mon Sep 17 00:00:00 2001
> > From: Anoop C S <anoopcs at redhat.com>
> > Date: Sat, 18 Aug 2018 14:29:05 +0530
> > Subject: [PATCH 1/2] s3/smbd: Fix a typo
> > 
> > Signed-off-by: Anoop C S <anoopcs at redhat.com>
> > ---
> >  source3/smbd/filename.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/source3/smbd/filename.c b/source3/smbd/filename.c
> > index 9e15af1916d..662c6a95fc1 100644
> > --- a/source3/smbd/filename.c
> > +++ b/source3/smbd/filename.c
> > @@ -183,7 +183,7 @@ static NTSTATUS check_parent_exists(TALLOC_CTX *ctx,
> >  
> >  	/*
> >  	 * If there was no parent component in
> > -	 * smb_fname->base_name of the parent name
> > +	 * smb_fname->base_name or the parent name
> >  	 * contained a wildcard then don't do this
> >  	 * optimization.
> >  	 */
> > -- 
> > 2.17.1
> > 
> > 
> > From 9d1cadd230841ffd18ab5629e3a3c84c86d3ac34 Mon Sep 17 00:00:00 2001
> > From: Anoop C S <anoopcs at redhat.com>
> > Date: Sat, 18 Aug 2018 15:19:15 +0530
> > Subject: [PATCH 2/2] s3/smbd: Update dirpath in missing places inside
> >  check_parent_exists()
> > 
> > We failed to update *pp_dirpath in the following situations:
> > * if smb_fname does not contain a '/'.
> >   - which essentially implies a file/directory under root of the share
> >     while we are at it. dir_path is expected to be '.'
> > * if stat of parent_fname fails
> > 
> > Signed-off-by: Anoop C S <anoopcs at redhat.com>
> > ---
> >  source3/smbd/filename.c | 19 ++++++++++++-------
> >  1 file changed, 12 insertions(+), 7 deletions(-)
> > 
> > diff --git a/source3/smbd/filename.c b/source3/smbd/filename.c
> > index 662c6a95fc1..6a8177cbb49 100644
> > --- a/source3/smbd/filename.c
> > +++ b/source3/smbd/filename.c
> > @@ -174,7 +174,8 @@ static NTSTATUS check_parent_exists(TALLOC_CTX *ctx,
> >  	if (!parent_dirname(ctx, smb_fname->base_name,
> >  				&parent_fname.base_name,
> >  				&last_component)) {
> > -		return NT_STATUS_NO_MEMORY;
> > +		status = NT_STATUS_NO_MEMORY;
> > +		goto err;
> >  	}
> >  
> >  	if (!posix_pathnames) {
> > @@ -189,7 +190,8 @@ static NTSTATUS check_parent_exists(TALLOC_CTX *ctx,
> >  	 */
> >  	if ((smb_fname->base_name == last_component) ||
> >  			parent_fname_has_wild) {
> > -		return NT_STATUS_OK;
> > +		status = NT_STATUS_OK;
> > +		goto done;
> >  	}
> >  
> >  	if (posix_pathnames) {
> > @@ -202,14 +204,16 @@ static NTSTATUS check_parent_exists(TALLOC_CTX *ctx,
> >  	   with the normal tree walk. */
> >  
> >  	if (ret == -1) {
> > -		return NT_STATUS_OK;
> > +		status = NT_STATUS_OK;
> > +		goto done;
> >  	}
> >  
> >  	status = check_for_dot_component(&parent_fname);
> >  	if (!NT_STATUS_IS_OK(status)) {
> > -		return status;
> > +		goto err;
> >  	}
> >  
> > +done:
> >  	/* Parent exists - set "start" to be the
> >  	 * last component to shorten the tree walk. */
> >  
> > @@ -224,7 +228,8 @@ static NTSTATUS check_parent_exists(TALLOC_CTX *ctx,
> >  	TALLOC_FREE(*pp_dirpath);
> >  	*pp_dirpath = talloc_strdup(ctx, parent_fname.base_name);
> >  	if (!*pp_dirpath) {
> > -		return NT_STATUS_NO_MEMORY;
> > +		status = NT_STATUS_NO_MEMORY;
> > +		goto err;
> >  	}
> >  
> >  	DEBUG(5,("check_parent_exists: name "
> > @@ -233,8 +238,8 @@ static NTSTATUS check_parent_exists(TALLOC_CTX *ctx,
> >  		smb_fname->base_name,
> >  		*pp_dirpath,
> >  		*pp_start));
> > -
> > -	return NT_STATUS_OK;
> > +err:
> > +	return status;
> >  }
> >  
> >  /*
> > -- 
> > 2.17.1
> > 
> 
>