diff --git a/samba-smbclient-mget-bug-14517.patch b/samba-smbclient-mget-bug-14517.patch new file mode 100644 index 0000000..8f21623 --- /dev/null +++ b/samba-smbclient-mget-bug-14517.patch @@ -0,0 +1,430 @@ +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 + diff --git a/samba.spec b/samba.spec index f76af7a..b8593e5 100644 --- a/samba.spec +++ b/samba.spec @@ -132,6 +132,7 @@ Source14: samba.pamd Source201: README.downgrade Patch1: samba-s4u.patch Patch2: samba-4.13-redhat.patch +Patch3: samba-smbclient-mget-bug-14517.patch Requires(pre): /usr/sbin/groupadd Requires(post): systemd @@ -3617,6 +3618,7 @@ fi %changelog * Wed Nov 25 2020 Alexander Bokovoy - 4.13.2-2 +- rhbz#1892745, rhbz#1900232: smbclient mget crashes (upstream bug 14517) - Merge RHEL 8.4 patches: - FIPS-related enhancements - FreeIPA Global Catalog patches