[PATCH] Fix failure to update dirpath

[Anoop C S]

On Mon, 2018-08-20 at 16:20 -0700, Jeremy Allison wrote:
> On Tue, Aug 21, 2018 at 04:29:21AM +0530, Anoop C S wrote:
> > 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
> 
> Ah, got it - thanks for the explaination.
> 
> The thing I missed is that parent_dirname() allocates
> the ".", and that on any return of NT_STATUS_OK then
> pp_dirpath and pp_start must have been allocated.
> 
> I don't think your patch is quite right though.
> 
> The error in your fix is when:
> 
> parent_fname_has_wild = ms_has_wild(parent_fname.base_name)
> 
> is true. At that point, parent_fname.base_name != ".",
> and we can't just return via this:
> 
>         *pp_start = discard_const_p(char, last_component);
> 
>         /* Update dirpath. */
>         TALLOC_FREE(*pp_dirpath);
>         *pp_dirpath = talloc_strdup(ctx, parent_fname.base_name);
> 
> as we'll miss the wildcard in the parent name. The correct
> fix should ensure we return:
> 
> *pp_dirpath = "."
> *pp_start = smb_fname->base_name
> 
> in that case. So we need a slightly more complex fix. Give
> me a little time to figure it out. Can you log a bug
> for this though, so we can track it ?

Done.

https://bugzilla.samba.org/show_bug.cgi?id=13585

> Thanks,
> 
> Jeremy.
>