[PATCH] Fix failure to update dirpath

[Jeremy Allison]

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 ?

Thanks,

Jeremy.