The Samba-Bugzilla – Attachment 16267 Details for
Bug 14517
smbclient recursive mget crashes
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[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:
Patch for 4.13
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 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Raw
Flags:
vl
:
review?
(
slow
)
jra
:
review+
vl
:
ci-passed+
Actions:
View
Attachments on
bug 14517
: 16267