Attachment 16267 Details for Bug 14517 – Patch for 4.13

[patch] Patch for 4.13

bug-14517.txt (text/plain), 12.79 KB, created by Volker Lendecke on 2020-10-01 10:46:47 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Volker Lendecke

Created: 2020-10-01 10:46:47 UTC

Size: 12.79 KB

patch

obsolete

>From 52ddfacead1ba50da0fc706b54e90e7a0cadb8e9 Mon Sep 17 00:00:00 2001
>From: Volker Lendecke <vl@samba.org>
>Date: Mon, 28 Sep 2020 14:11:13 +0200
>Subject: [PATCH 1/4] smbclient: Remove the "abort_mget" variable
>
>This was never set to true anywhere in the code
>
>Bug: https://bugzilla.samba.org/show_bug.cgi?id=14517
>Signed-off-by: Volker Lendecke <vl@samba.org>
>Reviewed-by: Jeremy Allison <jra@samba.org>
>(cherry picked from commit 8fa451d2b052223a11b24ffc2a956b80d03aaa7c)
>---
> source3/client/client.c | 9 ---------
> 1 file changed, 9 deletions(-)
>
>diff --git a/source3/client/client.c b/source3/client/client.c
>index f65293849d0..5bed37fc2a2 100644
>--- a/source3/client/client.c
>+++ b/source3/client/client.c
>@@ -87,8 +87,6 @@ static char dest_ss_str[INET6_ADDRSTRLEN];
> 
> #define SEPARATORS " \t\n\r"
> 
>-static bool abort_mget = true;
>-
> /* timing globals */
> uint64_t get_total_size = 0;
> unsigned int get_total_time_ms = 0;
>@@ -1217,11 +1215,6 @@ static NTSTATUS do_mget(struct cli_state *cli_state, struct file_info *finfo,
> 	if (strequal(finfo->name,".") || strequal(finfo->name,".."))
> 		return NT_STATUS_OK;
> 
>-	if (abort_mget)	{
>-		d_printf("mget aborted\n");
>-		return NT_STATUS_UNSUCCESSFUL;
>-	}
>-
> 	if (finfo->attr & FILE_ATTRIBUTE_DIRECTORY) {
> 		if (asprintf(&quest,
> 			 "Get directory %s? ",finfo->name) < 0) {
>@@ -1419,8 +1412,6 @@ static int cmd_mget(void)
> 		attribute |= FILE_ATTRIBUTE_DIRECTORY;
> 	}
> 
>-	abort_mget = false;
>-
> 	while (next_token_talloc(ctx, &cmd_ptr,&buf,NULL)) {
> 
> 		mget_mask = talloc_strdup(ctx, client_get_cur_dir());
>-- 
>2.20.1
>
>
>From 159a03a9067f7aeddb29080dc34e37b567a02479 Mon Sep 17 00:00:00 2001
>From: Volker Lendecke <vl@samba.org>
>Date: Mon, 28 Sep 2020 14:21:24 +0200
>Subject: [PATCH 2/4] smbclient: Slightly simplify do_mget()
>
>Put the prompt query into a separate if-statement, move the "quest"
>variable closer to its use
>
>Bug: https://bugzilla.samba.org/show_bug.cgi?id=14517
>Signed-off-by: Volker Lendecke <vl@samba.org>
>Reviewed-by: Jeremy Allison <jra@samba.org>
>(cherry picked from commit 71bc4d4b8d94458ac2e40d659f06110d434fd5c9)
>---
> source3/client/client.c | 28 ++++++++++++++--------------
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
>diff --git a/source3/client/client.c b/source3/client/client.c
>index 5bed37fc2a2..5901419f427 100644
>--- a/source3/client/client.c
>+++ b/source3/client/client.c
>@@ -1203,7 +1203,6 @@ static NTSTATUS do_mget(struct cli_state *cli_state, struct file_info *finfo,
> 	TALLOC_CTX *ctx = talloc_tos();
> 	NTSTATUS status = NT_STATUS_OK;
> 	char *rname = NULL;
>-	char *quest = NULL;
> 	char *saved_curdir = NULL;
> 	char *mget_mask = NULL;
> 	char *new_cd = NULL;
>@@ -1215,23 +1214,24 @@ static NTSTATUS do_mget(struct cli_state *cli_state, struct file_info *finfo,
> 	if (strequal(finfo->name,".") || strequal(finfo->name,".."))
> 		return NT_STATUS_OK;
> 
>-	if (finfo->attr & FILE_ATTRIBUTE_DIRECTORY) {
>-		if (asprintf(&quest,
>-			 "Get directory %s? ",finfo->name) < 0) {
>-			return NT_STATUS_NO_MEMORY;
>-		}
>-	} else {
>-		if (asprintf(&quest,
>-			 "Get file %s? ",finfo->name) < 0) {
>+	if (prompt) {
>+		const char *object = (finfo->attr & FILE_ATTRIBUTE_DIRECTORY) ?
>+			"directory" : "file";
>+		char *quest = NULL;
>+		bool ok;
>+
>+		quest = talloc_asprintf(
>+			ctx, "Get %s %s? ", object, finfo->name);
>+		if (quest == NULL) {
> 			return NT_STATUS_NO_MEMORY;
> 		}
>-	}
> 
>-	if (prompt && !yesno(quest)) {
>-		SAFE_FREE(quest);
>-		return NT_STATUS_OK;
>+		ok = yesno(quest);
>+		TALLOC_FREE(quest);
>+		if (!ok) {
>+			return NT_STATUS_OK;
>+		}
> 	}
>-	SAFE_FREE(quest);
> 
> 	if (!(finfo->attr & FILE_ATTRIBUTE_DIRECTORY)) {
> 		rname = talloc_asprintf(ctx,
>-- 
>2.20.1
>
>
>From 523ccc98d2c6a9ddc0714084b5e19cee2a80bf27 Mon Sep 17 00:00:00 2001
>From: Volker Lendecke <vl@samba.org>
>Date: Mon, 28 Sep 2020 16:29:27 +0200
>Subject: [PATCH 3/4] test3: Add a test showing that smbclient recursive mget
> is broken
>
>Bug: https://bugzilla.samba.org/show_bug.cgi?id=14517
>Signed-off-by: Volker Lendecke <vl@samba.org>
>Reviewed-by: Jeremy Allison <jra@samba.org>
>(cherry picked from commit 254a5b034e5a081c9d3f28717a4b54d2af0180fc)
>---
> selftest/knownfail.d/smbclient_mget         |  1 +
> source3/script/tests/test_smbclient_mget.sh | 39 +++++++++++++++++++++
> source3/selftest/tests.py                   | 10 ++++++
> 3 files changed, 50 insertions(+)
> create mode 100644 selftest/knownfail.d/smbclient_mget
> create mode 100755 source3/script/tests/test_smbclient_mget.sh
>
>diff --git a/selftest/knownfail.d/smbclient_mget b/selftest/knownfail.d/smbclient_mget
>new file mode 100644
>index 00000000000..64407a8c5d4
>--- /dev/null
>+++ b/selftest/knownfail.d/smbclient_mget
>@@ -0,0 +1 @@
>+^samba3.blackbox.smbclient-mget.smbclient\ mget$fileserver$
>\ No newline at end of file
>diff --git a/source3/script/tests/test_smbclient_mget.sh b/source3/script/tests/test_smbclient_mget.sh
>new file mode 100755
>index 00000000000..45f62f15d4d
>--- /dev/null
>+++ b/source3/script/tests/test_smbclient_mget.sh
>@@ -0,0 +1,39 @@
>+#!/bin/sh
>+
>+if [ $# -lt 6 ]; then
>+cat <<EOF
>+Usage: $0 smbclient3 server share user password directory
>+EOF
>+exit 1;
>+fi
>+
>+incdir=`dirname $0`/../../../testprogs/blackbox
>+. $incdir/subunit.sh
>+
>+failed=0
>+
>+SMBCLIENT3="$1"; shift
>+SERVER="$1"; shift
>+SHARE="$1"; shift
>+USERNAME="$1"; shift
>+PASSWORD="$1"; shift
>+DIRECTORY="$1"; shift
>+
>+# Can't use "testit" here -- it somehow breaks the -c command passed
>+# to smbclient into two, spoiling the "mget"
>+
>+name="smbclient mget"
>+subunit_start_test "$name"
>+output=$("$SMBCLIENT3" //"$SERVER"/"$SHARE" \
>+         -U"$USERNAME"%"$PASSWORD" -c "recurse;prompt;mget $DIRECTORY")
>+status=$?
>+if [ x$status = x0 ]; then
>+    subunit_pass_test "$name"
>+else
>+    echo "$output" | subunit_fail_test "$name"
>+fi
>+
>+testit "rm foo" rm "$DIRECTORY"/foo || failed=`expr $failed + 1`
>+testit "rmdir $DIRECTORY" rmdir "$DIRECTORY" || failed=`expr $failed + 1`
>+
>+testok $0 $failed
>diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
>index d05de6bd08c..f9202f3f93a 100755
>--- a/source3/selftest/tests.py
>+++ b/source3/selftest/tests.py
>@@ -1098,6 +1098,16 @@ for env in ["ad_member_idmap_rid:local", "maptoguest:local"]:
> 
> plantestsuite("samba3.blackbox.itime", "ad_dc", [os.path.join(samba3srcdir, "script/tests/test_itime.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$LOCAL_PATH', smbclient3, 'xattr'])
> 
>+plantestsuite("samba3.blackbox.smbclient-mget",
>+              "fileserver",
>+              [os.path.join(samba3srcdir, "script/tests/test_smbclient_mget.sh"),
>+               smbclient3,
>+               "$SERVER",
>+               "tmp",
>+               "$USERNAME",
>+               "$PASSWORD",
>+               "valid_users"])
>+
> t = "readdir-timestamp"
> plantestsuite(
>     "samba3.smbtorture_s3.plain.%s" % t,
>-- 
>2.20.1
>
>
>From 8cf00e6d64b098c8c21656e9f56d389758503dcd Mon Sep 17 00:00:00 2001
>From: Volker Lendecke <vl@samba.org>
>Date: Mon, 28 Sep 2020 15:03:41 +0200
>Subject: [PATCH 4/4] smbclient: Fix recursive mget
>
>Make do_mget rely on do_list() already doing the recursion in a
>breadth-first manner. The previous code called do_list() from within
>its callback. Unfortunately the recent simplifications of do_list()
>broke this, leading to recursive mget to segfault. Instead of figuring
>out how this worked before the simplifications in do_list() (I did
>spend a few hours on this) and fixing it, I chose to restructure
>do_mget() to not recursively call do_list() anymore but instead rely
>on do_list() to do the recursion. Saves quite a few lines of code and
>complexity.
>
>Bug: https://bugzilla.samba.org/show_bug.cgi?id=14517
>Signed-off-by: Volker Lendecke <vl@samba.org>
>Reviewed-by: Jeremy Allison <jra@samba.org>
>
>Autobuild-User(master): Jeremy Allison <jra@samba.org>
>Autobuild-Date(master): Wed Sep 30 17:23:45 UTC 2020 on sn-devel-184
>
>(cherry picked from commit 9f24b5098f796f364a3f403ad4e9ae28b3c0935a)
>---
> selftest/knownfail.d/smbclient_mget |   1 -
> source3/client/client.c             | 121 ++++++++--------------------
> 2 files changed, 33 insertions(+), 89 deletions(-)
> delete mode 100644 selftest/knownfail.d/smbclient_mget
>
>diff --git a/selftest/knownfail.d/smbclient_mget b/selftest/knownfail.d/smbclient_mget
>deleted file mode 100644
>index 64407a8c5d4..00000000000
>--- a/selftest/knownfail.d/smbclient_mget
>+++ /dev/null
>@@ -1 +0,0 @@
>-^samba3.blackbox.smbclient-mget.smbclient\ mget$fileserver$
>\ No newline at end of file
>diff --git a/source3/client/client.c b/source3/client/client.c
>index 5901419f427..8c7ceb644aa 100644
>--- a/source3/client/client.c
>+++ b/source3/client/client.c
>@@ -1201,11 +1201,10 @@ static NTSTATUS do_mget(struct cli_state *cli_state, struct file_info *finfo,
> 		    const char *dir)
> {
> 	TALLOC_CTX *ctx = talloc_tos();
>-	NTSTATUS status = NT_STATUS_OK;
>-	char *rname = NULL;
>-	char *saved_curdir = NULL;
>-	char *mget_mask = NULL;
>-	char *new_cd = NULL;
>+	const char *client_cwd = NULL;
>+	size_t client_cwd_len;
>+	char *path = NULL;
>+	char *local_path = NULL;
> 
> 	if (!finfo->name) {
> 		return NT_STATUS_OK;
>@@ -1214,6 +1213,10 @@ static NTSTATUS do_mget(struct cli_state *cli_state, struct file_info *finfo,
> 	if (strequal(finfo->name,".") || strequal(finfo->name,".."))
> 		return NT_STATUS_OK;
> 
>+	if ((finfo->attr & FILE_ATTRIBUTE_DIRECTORY) && !recurse) {
>+		return NT_STATUS_OK;
>+	}
>+
> 	if (prompt) {
> 		const char *object = (finfo->attr & FILE_ATTRIBUTE_DIRECTORY) ?
> 			"directory" : "file";
>@@ -1233,98 +1236,40 @@ static NTSTATUS do_mget(struct cli_state *cli_state, struct file_info *finfo,
> 		}
> 	}
> 
>-	if (!(finfo->attr & FILE_ATTRIBUTE_DIRECTORY)) {
>-		rname = talloc_asprintf(ctx,
>-				"%s%s",
>-				client_get_cur_dir(),
>-				finfo->name);
>-		if (!rname) {
>-			return NT_STATUS_NO_MEMORY;
>-		}
>-		rname = client_clean_name(ctx, rname);
>-		if (rname == NULL) {
>-			return NT_STATUS_NO_MEMORY;
>-		}
>-		do_get(rname, finfo->name, false);
>-		TALLOC_FREE(rname);
>-		return NT_STATUS_OK;
>-	}
>-
>-	/* handle directories */
>-	saved_curdir = talloc_strdup(ctx, client_get_cur_dir());
>-	if (!saved_curdir) {
>+	path = talloc_asprintf(
>+		ctx, "%s%c%s", dir, CLI_DIRSEP_CHAR, finfo->name);
>+	if (path == NULL) {
> 		return NT_STATUS_NO_MEMORY;
> 	}
>-
>-	new_cd = talloc_asprintf(ctx,
>-				"%s%s%s",
>-				client_get_cur_dir(),
>-				finfo->name,
>-				CLI_DIRSEP_STR);
>-	if (!new_cd) {
>-		return NT_STATUS_NO_MEMORY;
>-	}
>-	new_cd = client_clean_name(ctx, new_cd);
>-	if (new_cd == NULL) {
>+	path = client_clean_name(ctx, path);
>+	if (path == NULL) {
> 		return NT_STATUS_NO_MEMORY;
> 	}
>-	client_set_cur_dir(new_cd);
>-
>-	string_replace(finfo->name,'\\','/');
>-	if (lowercase) {
>-		if (!strlower_m(finfo->name)) {
>-			return NT_STATUS_INVALID_PARAMETER;
>-		}
>-	}
>-
>-	if (!directory_exist(finfo->name) &&
>-	    mkdir(finfo->name,0777) != 0) {
>-		d_printf("failed to create directory %s\n",finfo->name);
>-		client_set_cur_dir(saved_curdir);
>-		return map_nt_error_from_unix(errno);
>-	}
>-
>-	if (chdir(finfo->name) != 0) {
>-		d_printf("failed to chdir to directory %s\n",finfo->name);
>-		client_set_cur_dir(saved_curdir);
>-		return map_nt_error_from_unix(errno);
>-	}
> 
>-	mget_mask = talloc_asprintf(ctx,
>-			"%s*",
>-			client_get_cur_dir());
>+	/*
>+	 * Skip the path prefix if we've done a remote "cd" when
>+	 * creating the local path
>+	 */
>+	client_cwd = client_get_cur_dir();
>+	client_cwd_len = strlen(client_cwd);
> 
>-	if (!mget_mask) {
>+	local_path = talloc_strdup(ctx, path + client_cwd_len);
>+	if (local_path == NULL) {
>+		TALLOC_FREE(path);
> 		return NT_STATUS_NO_MEMORY;
> 	}
>+	string_replace(local_path, CLI_DIRSEP_CHAR, '/');
> 
>-	mget_mask = client_clean_name(ctx, mget_mask);
>-	if (mget_mask == NULL) {
>-		return NT_STATUS_NO_MEMORY;
>-	}
>-	status = do_list(mget_mask,
>-			 (FILE_ATTRIBUTE_SYSTEM
>-			  | FILE_ATTRIBUTE_HIDDEN
>-			  | FILE_ATTRIBUTE_DIRECTORY),
>-			 do_mget, false, true);
>-	if (!NT_STATUS_IS_OK(status)
>-	 && !NT_STATUS_EQUAL(status, NT_STATUS_ACCESS_DENIED)) {
>-		/*
>-		 * Ignore access denied errors to ensure all permitted files are
>-		 * pulled down.
>-		 */
>-		return status;
>-	}
>+	if (finfo->attr & FILE_ATTRIBUTE_DIRECTORY) {
>+		int ret = mkdir(local_path, 0777);
> 
>-	if (chdir("..") == -1) {
>-		d_printf("do_mget: failed to chdir to .. (error %s)\n",
>-			strerror(errno) );
>-		return map_nt_error_from_unix(errno);
>+		if ((ret == -1) && (errno != EEXIST)) {
>+			return map_nt_error_from_unix(errno);
>+		}
>+	} else {
>+		do_get(path, local_path, false);
> 	}
>-	client_set_cur_dir(saved_curdir);
>-	TALLOC_FREE(mget_mask);
>-	TALLOC_FREE(saved_curdir);
>-	TALLOC_FREE(new_cd);
>+
> 	return NT_STATUS_OK;
> }
> 
>@@ -1431,7 +1376,7 @@ static int cmd_mget(void)
> 		if (mget_mask == NULL) {
> 			return 1;
> 		}
>-		status = do_list(mget_mask, attribute, do_mget, false, true);
>+		status = do_list(mget_mask, attribute, do_mget, recurse, true);
> 		if (!NT_STATUS_IS_OK(status)) {
> 			return 1;
> 		}
>@@ -1453,7 +1398,7 @@ static int cmd_mget(void)
> 		if (mget_mask == NULL) {
> 			return 1;
> 		}
>-		status = do_list(mget_mask, attribute, do_mget, false, true);
>+		status = do_list(mget_mask, attribute, do_mget, recurse, true);
> 		if (!NT_STATUS_IS_OK(status)) {
> 			return 1;
> 		}
>-- 
>2.20.1
>

Flags:

vl: review? (slow)
jra: review+
vl: ci-passed+

Actions: View

Attachments on bug 14517: 16267