[PATCH] Fix failure to update dirpath
Michael Adam
obnox at samba.org
Wed Aug 22 14:35:16 UTC 2018
On 2018-08-21 at 09:54 -0700, Jeremy Allison via samba-technical wrote:
> On Tue, Aug 21, 2018 at 11:42:40AM +0530, Anoop C S wrote:
> > On Mon, 2018-08-20 at 18:35 -0700, Jeremy Allison wrote:
> > > On Mon, Aug 20, 2018 at 06:02:31PM -0700, Jeremy Allison via samba-technical wrote:
> > > >
> > > > Hang on a minute. I've been looking really closely
> > > > now at the *callers* of check_parent_exists() and
> > > > many of them depend on *dirpath == '\0' in the
> > > > case where smb_fname->base_name doesn't contain
> > > > a component containing '/' characters.
> > > >
> > > > I think my fix, although it might work, will affect
> > > > other paths. Let me look at this more closely
> > > > tomorrow. I'm withdrawing this for now.
> > >
> > > OK, my fix was way too invasive for the fix that
> > > you need to make Gluster work again for that bug.
> > >
> > > The problem is that the code that constructs
> > > the complete pathname depends on *dirpath == '\0'
> > > to avoid adding a "./" prefix to every constructed
> > > pathname. My patch would cause all of those
> > > code paths to change to the full/component/path
> > > cases, which is not what we want. That's going
> > > to affect what goes into the stat-cache as well
> > > as many other things I'll have to go through
> > > carefully to find.
> >
> > Ok.
> >
> > > This is a set of code-paths that we might clean
> > > up in the future, but not for this specific bug
> > > I think.
>
> FYI, the attached diff is the full clean-up
> that is required to make everything
> work correctly (otherwise it fails in the
> samba3.blackbox.smbclient_s3.SMB3.plain
> test).
>
> I'm not planning to propose this as it's a
> more invasive change than we need, but I wanted
> to make sure I fully understood what the
> issue was :-).
I do actually think that this is conceptionally
the much better patch. It fixes the slightly
arbitrary (and undocumented) behavior to return
"" for dirpath in the non-optimized case and had
all callers react to that arbitrary fact. But the
dirpath was not really useful for other processing.
With this change you always return a valid
dirpath, and we don't need to change it
(to ".") for some callers...
A few more cosmetic review commments below in the patch.
> > > Can you test the following change instead ?
> >
> > The attached diff also works.
>
> Thanks for testing !
>
> > > Much simpler and I think it will fix the immediate
> > > Gluster problem (it's the same change that already
> > > exists inside get_real_filename_full_scan() so
> > > I think this is something we already ran across
> > > there).
> >
> > Considering the fact that we are not 100% sure about the side effects of previous fix which was more
> > aggressive, the current solution is more clean and elegant. In addition to that the presence of same
> > statements inside get_real_filename_full_scan(), which is a fallback, gives us more confidence to
> > fix it here.
> >
> > I hope you you will be attaching the diff as a patch to other thread for review.
>
> Yep, I'll post the "simple" patch as the
> proposed fix for bug:
>
> https://bugzilla.samba.org/show_bug.cgi?id=13585
>
> in another thread.
>
> Thanks for all your help !
>
> Cheers,
>
> Jeremy.
> From 06674319f82e5ee26a71ff16018b27f64fdf98f0 Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra at samba.org>
> Date: Mon, 20 Aug 2018 16:51:18 -0700
> Subject: [PATCH] s3: smbd: Ensure check_parent_exists() returns valid paths
> for non-optimization return.
>
> Needed for vfs_glusterfs, as Gluster requires ".".
>
> Based on a fix from Anoop C S <anoopcs at redhat.com>
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13585
>
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
> source3/smbd/filename.c | 54 ++++++++++++++++++++++++++++-------------
> 1 file changed, 37 insertions(+), 17 deletions(-)
>
> diff --git a/source3/smbd/filename.c b/source3/smbd/filename.c
> index 9e15af1916d..3f73b6713df 100644
> --- a/source3/smbd/filename.c
> +++ b/source3/smbd/filename.c
> @@ -164,13 +164,11 @@ static NTSTATUS check_parent_exists(TALLOC_CTX *ctx,
> char **pp_dirpath,
> char **pp_start)
> {
> - struct smb_filename parent_fname;
> + struct smb_filename parent_fname = { 0 };
[1] This is an unrelated cleanup and should be a separate patch.
> const char *last_component = NULL;
> NTSTATUS status;
> int ret;
> - bool parent_fname_has_wild = false;
>
> - ZERO_STRUCT(parent_fname);
Together with [1] above.
> if (!parent_dirname(ctx, smb_fname->base_name,
> &parent_fname.base_name,
> &last_component)) {
> @@ -178,18 +176,18 @@ static NTSTATUS check_parent_exists(TALLOC_CTX *ctx,
> }
>
> if (!posix_pathnames) {
> - parent_fname_has_wild = ms_has_wild(parent_fname.base_name);
[2] This change of removing the variable and intead adding a return
(or late the goto) is also a patch that is an independent
code cleanup and should imho be a separate patch.
> + if (ms_has_wild(parent_fname.base_name)) {
> + goto no_optimization_out;
> + }
> }
>
> /*
> * If there was no parent component in
> - * smb_fname->base_name of the parent name
> - * contained a wildcard then don't do this
> + * smb_fname->base_name then don't do this
> * optimization.
> */
> - if ((smb_fname->base_name == last_component) ||
> - parent_fname_has_wild) {
Together with [2] above.
Only the change from return to goto should go into
the actual fix patch. (all in one patchset though).
Apart from these comments, the patch LGTM.
Thanks - Michael
> - return NT_STATUS_OK;
> + if (smb_fname->base_name == last_component) {
> + goto no_optimization_out;
> }
>
> if (posix_pathnames) {
> @@ -202,7 +200,7 @@ static NTSTATUS check_parent_exists(TALLOC_CTX *ctx,
> with the normal tree walk. */
>
> if (ret == -1) {
> - return NT_STATUS_OK;
> + goto no_optimization_out;
> }
>
> status = check_for_dot_component(&parent_fname);
> @@ -235,6 +233,28 @@ static NTSTATUS check_parent_exists(TALLOC_CTX *ctx,
> *pp_start));
>
> return NT_STATUS_OK;
> +
> + no_optimization_out:
> +
> + /*
> + * We must still return an *pp_dirpath
> + * initialized to ".", and a *pp_start
> + * pointing at smb_fname->base_name.
> + */
> +
> + TALLOC_FREE(parent_fname.base_name);
> +
> + *pp_dirpath = talloc_strdup(ctx, ".");
> + if (*pp_dirpath == NULL) {
> + return NT_STATUS_NO_MEMORY;
> + }
> + /*
> + * Safe to use discard_const_p
> + * here as by convention smb_fname->base_name
> + * is allocated off ctx.
> + */
> + *pp_start = discard_const_p(char, smb_fname->base_name);
> + return NT_STATUS_OK;
> }
>
> /*
> @@ -599,7 +619,7 @@ NTSTATUS unix_convert(TALLOC_CTX *ctx,
> goto err;
> }
> /* dirpath must exist. */
> - dirpath = talloc_strdup(ctx,"");
> + dirpath = talloc_strdup(ctx,".");
> if (dirpath == NULL) {
> status = NT_STATUS_NO_MEMORY;
> goto err;
> @@ -636,7 +656,7 @@ NTSTATUS unix_convert(TALLOC_CTX *ctx,
> * building the directories with talloc_asprintf and free it.
> */
>
> - if ((dirpath == NULL) && (!(dirpath = talloc_strdup(ctx,"")))) {
> + if ((dirpath == NULL) && (!(dirpath = talloc_strdup(ctx,".")))) {
> DEBUG(0, ("talloc_strdup failed\n"));
> status = NT_STATUS_NO_MEMORY;
> goto err;
> @@ -1036,7 +1056,7 @@ NTSTATUS unix_convert(TALLOC_CTX *ctx,
> size_t start_ofs =
> start - smb_fname->base_name;
>
> - if (*dirpath != '\0') {
> + if (!ISDOT(dirpath)) {
> tmp = talloc_asprintf(
> smb_fname, "%s/%s",
> dirpath, unmangled);
> @@ -1071,7 +1091,7 @@ NTSTATUS unix_convert(TALLOC_CTX *ctx,
> size_t start_ofs =
> start - smb_fname->base_name;
>
> - if (*dirpath != '\0') {
> + if (!ISDOT(dirpath)) {
> tmp = talloc_asprintf(smb_fname,
> "%s/%s/%s", dirpath,
> found_name, end+1);
> @@ -1096,7 +1116,7 @@ NTSTATUS unix_convert(TALLOC_CTX *ctx,
> size_t start_ofs =
> start - smb_fname->base_name;
>
> - if (*dirpath != '\0') {
> + if (!ISDOT(dirpath)) {
> tmp = talloc_asprintf(smb_fname,
> "%s/%s", dirpath,
> found_name);
> @@ -1137,7 +1157,7 @@ NTSTATUS unix_convert(TALLOC_CTX *ctx,
> * Add to the dirpath that we have resolved so far.
> */
>
> - if (*dirpath != '\0') {
> + if (!ISDOT(dirpath)) {
> char *tmp = talloc_asprintf(ctx,
> "%s/%s", dirpath, start);
> if (!tmp) {
> @@ -1207,7 +1227,7 @@ NTSTATUS unix_convert(TALLOC_CTX *ctx,
> return NT_STATUS_OK;
> fail:
> DEBUG(10, ("dirpath = [%s] start = [%s]\n", dirpath, start));
> - if (dirpath && *dirpath != '\0') {
> + if (dirpath && !ISDOT(dirpath)) {
> smb_fname->base_name = talloc_asprintf(smb_fname, "%s/%s",
> dirpath, start);
> } else {
> --
> 2.18.0.865.gffc8e1a3cd6-goog
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 163 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20180822/84682178/signature.sig>
More information about the samba-technical
mailing list