[PATCH] Fix failure to update dirpath
Anoop C S
anoopcs at autistici.org
Mon Aug 20 23:21:38 UTC 2018
On Tue, 2018-08-21 at 04:29 +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.
To be more precise, parent_dirname() inside check_parent_exists() correctly puts "." into *parent
(which is nothing but parent_fname.base_name). It is just that we did not copy that over to
*pp_dirpath before returning from check_parent_exists().
> 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
> > >
> >
> >
More information about the samba-technical
mailing list