From 52ddfacead1ba50da0fc706b54e90e7a0cadb8e9 Mon Sep 17 00:00:00 2001 From: Volker Lendecke 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 Reviewed-by: Jeremy Allison (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 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 Reviewed-by: Jeremy Allison (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 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 Reviewed-by: Jeremy Allison (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 < 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 Reviewed-by: Jeremy Allison Autobuild-User(master): Jeremy Allison 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