[PATCH] Fix failure to update dirpath
Jeremy Allison
jra at samba.org
Tue Aug 28 21:08:04 UTC 2018
On Wed, Aug 22, 2018 at 01:51:00PM -0700, Jeremy Allison wrote:
> On Wed, Aug 22, 2018 at 04:35:16PM +0200, Michael Adam wrote:
> >
> > 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.
>
> OK, now the "simple" fix has gone in, here is the
> cleanup patchset for unix_convert() to ensure we
> always have a valid dirpath (incorporates your
> comments).
>
> Please review and let me know if it's OK.
Ping ! As you wanted it this way, please take
the time to review :-).
Thanks,
Jeremy.
> From b70a1b301f1dfba94b2aac314922ac4c017a2d2b Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra at samba.org>
> Date: Wed, 22 Aug 2018 09:09:27 -0700
> Subject: [PATCH 1/3] s3: smbd: Initialization cleanup.
>
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
> source3/smbd/filename.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/source3/smbd/filename.c b/source3/smbd/filename.c
> index 41c1710351e..2071ddc58d6 100644
> --- a/source3/smbd/filename.c
> +++ b/source3/smbd/filename.c
> @@ -164,13 +164,12 @@ 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};
> const char *last_component = NULL;
> NTSTATUS status;
> int ret;
> bool parent_fname_has_wild = false;
>
> - ZERO_STRUCT(parent_fname);
> if (!parent_dirname(ctx, smb_fname->base_name,
> &parent_fname.base_name,
> &last_component)) {
> --
> 2.18.0.1017.ga543ac7ca45-goog
>
>
> From 1354975735024324f098298344ea558cb6aaed8c Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra at samba.org>
> Date: Wed, 22 Aug 2018 09:15:12 -0700
> Subject: [PATCH 2/3] s3: smbd: Restructure check_parent_exists() to ensure all
> non-optimization returns go to one place.
>
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
> source3/smbd/filename.c | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/source3/smbd/filename.c b/source3/smbd/filename.c
> index 2071ddc58d6..479d532327a 100644
> --- a/source3/smbd/filename.c
> +++ b/source3/smbd/filename.c
> @@ -168,7 +168,6 @@ static NTSTATUS check_parent_exists(TALLOC_CTX *ctx,
> const char *last_component = NULL;
> NTSTATUS status;
> int ret;
> - bool parent_fname_has_wild = false;
>
> if (!parent_dirname(ctx, smb_fname->base_name,
> &parent_fname.base_name,
> @@ -177,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);
> + 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) {
> - return NT_STATUS_OK;
> + if (smb_fname->base_name == last_component) {
> + goto no_optimization_out;
> }
>
> if (posix_pathnames) {
> @@ -201,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);
> @@ -234,6 +233,10 @@ static NTSTATUS check_parent_exists(TALLOC_CTX *ctx,
> *pp_start));
>
> return NT_STATUS_OK;
> +
> + no_optimization_out:
> +
> + return NT_STATUS_OK;
> }
>
> /*
> --
> 2.18.0.1017.ga543ac7ca45-goog
>
>
> From ad6a950340767004e29545e4b44b22a2b9c249d0 Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra at samba.org>
> Date: Wed, 22 Aug 2018 13:37:04 -0700
> Subject: [PATCH 3/3] s3: smbd: Ensure dirpath is set to ".", not "\0" so it is
> always a valid path.
>
> Cleanup of the internals of unix_convert().
>
> Ensure check_parent_exists() returns this in the non-optimization
> case. Ensure unix_convert() initializes dirpath to ".".
>
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
> source3/smbd/filename.c | 32 +++++++++++++++++++++++++-------
> 1 file changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/source3/smbd/filename.c b/source3/smbd/filename.c
> index 479d532327a..16d0f340102 100644
> --- a/source3/smbd/filename.c
> +++ b/source3/smbd/filename.c
> @@ -236,6 +236,24 @@ static NTSTATUS check_parent_exists(TALLOC_CTX *ctx,
>
> 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;
> }
>
> @@ -601,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;
> @@ -638,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;
> @@ -1038,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);
> @@ -1073,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);
> @@ -1098,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);
> @@ -1139,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) {
> @@ -1209,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.1017.ga543ac7ca45-goog
>
More information about the samba-technical
mailing list