From 89217db6fa2421865df36783813a6d7d82488bbc Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Fri, 14 Jun 2024 13:40:35 +0200 Subject: [PATCH 1/9] test_recycle.sh: make sure we don't see panics on the log files BUG: https://bugzilla.samba.org/show_bug.cgi?id=15659 Signed-off-by: Stefan Metzmacher Reviewed-by: Martin Schwenke Reviewed-by: Noel Power Reviewed-by: Volker Lendecke (cherry picked from commit 2916b6096e16fb44d659b7e60d3f3a569d037279) --- source3/script/tests/test_recycle.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/source3/script/tests/test_recycle.sh b/source3/script/tests/test_recycle.sh index 8c9291feb926..ba1d0a598b14 100755 --- a/source3/script/tests/test_recycle.sh +++ b/source3/script/tests/test_recycle.sh @@ -90,11 +90,16 @@ quit return 0 } +panic_count_0=$(grep -c PANIC $SMBD_TEST_LOG) testit "recycle" \ test_recycle || failed=$((failed + 1)) +panic_count_1=$(grep -c PANIC $SMBD_TEST_LOG) + +testit "check_panic" test $panic_count_0 -eq $panic_count_1 || failed=$(expr $failed + 1) + # # Cleanup. do_cleanup -- 2.34.1 From 075cdfe4dc306b6ba46902f7aec2495ce3082648 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Fri, 14 Jun 2024 13:40:35 +0200 Subject: [PATCH 2/9] TMP-REPRODUCE: vfs_recycle: demonstrate memory corruption in recycle_unlink_internal() Forcing a reload of the smb.conf option values means the pointer learned in vfs_recycle_connect() become stale. This will be reverted at the end of the patset again. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15659 Signed-off-by: Stefan Metzmacher Reviewed-by: Martin Schwenke Reviewed-by: Noel Power Reviewed-by: Volker Lendecke (cherry picked from commit 6467c47cbe562e99e970dbb895e1068f54e6295b) --- selftest/knownfail.d/samba3.blackbox.recycle | 2 ++ source3/modules/vfs_recycle.c | 2 ++ 2 files changed, 4 insertions(+) create mode 100644 selftest/knownfail.d/samba3.blackbox.recycle diff --git a/selftest/knownfail.d/samba3.blackbox.recycle b/selftest/knownfail.d/samba3.blackbox.recycle new file mode 100644 index 000000000000..bae7f717e095 --- /dev/null +++ b/selftest/knownfail.d/samba3.blackbox.recycle @@ -0,0 +1,2 @@ +^samba3.blackbox.recycle.recycle.fileserver +^samba3.blackbox.recycle.check_panic.fileserver diff --git a/source3/modules/vfs_recycle.c b/source3/modules/vfs_recycle.c index 327a7eea06e3..43e229692d1d 100644 --- a/source3/modules/vfs_recycle.c +++ b/source3/modules/vfs_recycle.c @@ -437,6 +437,8 @@ static int recycle_unlink_internal(vfs_handle_struct *handle, int rc = -1; struct recycle_config_data *config; + reload_services(NULL, NULL, false); + SMB_VFS_HANDLE_GET_DATA(handle, config, struct recycle_config_data, -- 2.34.1 From 2ff6d38095582e79f986cd8c36998079f2bc0e7c Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Fri, 14 Jun 2024 10:07:02 +0200 Subject: [PATCH 3/9] vfs_recycle: don't unlink on allocation failure BUG: https://bugzilla.samba.org/show_bug.cgi?id=15659 Signed-off-by: Stefan Metzmacher Reviewed-by: Martin Schwenke Reviewed-by: Noel Power Reviewed-by: Volker Lendecke (cherry picked from commit 691564f6ca7d206939558b8e69b5fb86a3e68650) --- source3/modules/vfs_recycle.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/source3/modules/vfs_recycle.c b/source3/modules/vfs_recycle.c index 43e229692d1d..47b21983984f 100644 --- a/source3/modules/vfs_recycle.c +++ b/source3/modules/vfs_recycle.c @@ -650,13 +650,7 @@ static int recycle_unlink_internal(vfs_handle_struct *handle, TALLOC_FREE(smb_fname_final->base_name); smb_fname_final->base_name = talloc_strdup(smb_fname_final, final_name); - if (smb_fname_final->base_name == NULL) { - rc = SMB_VFS_NEXT_UNLINKAT(handle, - dirfsp, - smb_fname, - flags); - goto done; - } + ALLOC_CHECK(smb_fname_final->base_name, done); } DEBUG(10, ("recycle: Moving %s to %s\n", smb_fname_str_dbg(full_fname), -- 2.34.1 From f4a6f9e5bbe97b3ecb4aacb47881f5dc256a7fe7 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Fri, 14 Jun 2024 10:07:02 +0200 Subject: [PATCH 4/9] vfs_recycle: directly allocate smb_fname_final->base_name We can use talloc_asprintf() instead of asprintf() followed by talloc_strdup(). BUG: https://bugzilla.samba.org/show_bug.cgi?id=15659 Signed-off-by: Stefan Metzmacher Reviewed-by: Martin Schwenke Reviewed-by: Noel Power Reviewed-by: Volker Lendecke (cherry picked from commit 220b0e977e2e25f2033cfd62c17d998c750992fc) --- source3/modules/vfs_recycle.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/source3/modules/vfs_recycle.c b/source3/modules/vfs_recycle.c index 47b21983984f..e8c33c3585cf 100644 --- a/source3/modules/vfs_recycle.c +++ b/source3/modules/vfs_recycle.c @@ -643,14 +643,16 @@ static int recycle_unlink_internal(vfs_handle_struct *handle, /* rename file we move to recycle bin */ i = 1; while (recycle_file_exist(handle, smb_fname_final)) { - SAFE_FREE(final_name); - if (asprintf(&final_name, "%s/Copy #%d of %s", temp_name, i++, base) == -1) { - ALLOC_CHECK(final_name, done); - } + char *copy = NULL; + TALLOC_FREE(smb_fname_final->base_name); - smb_fname_final->base_name = talloc_strdup(smb_fname_final, - final_name); - ALLOC_CHECK(smb_fname_final->base_name, done); + copy = talloc_asprintf(smb_fname_final, "%s/Copy #%d of %s", + temp_name, i++, base); + if (copy == NULL) { + rc = -1; + goto done; + } + smb_fname_final->base_name = copy; } DEBUG(10, ("recycle: Moving %s to %s\n", smb_fname_str_dbg(full_fname), -- 2.34.1 From f47f3b4d1ef655dcba10417cdc8fbe24ff55c394 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Fri, 14 Jun 2024 10:07:02 +0200 Subject: [PATCH 5/9] vfs_recycle: use a talloc_stackframe() in recycle_unlink_internal() That makes the cleanup more clear... BUG: https://bugzilla.samba.org/show_bug.cgi?id=15659 Signed-off-by: Stefan Metzmacher Reviewed-by: Martin Schwenke Reviewed-by: Noel Power Reviewed-by: Volker Lendecke (cherry picked from commit cf7a6b521ac0bb903dabbd1af208d1af4fbe9a8b) --- source3/modules/vfs_recycle.c | 48 ++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/source3/modules/vfs_recycle.c b/source3/modules/vfs_recycle.c index e8c33c3585cf..7f54cef4127a 100644 --- a/source3/modules/vfs_recycle.c +++ b/source3/modules/vfs_recycle.c @@ -421,15 +421,16 @@ static int recycle_unlink_internal(vfs_handle_struct *handle, const struct smb_filename *smb_fname, int flags) { + TALLOC_CTX *frame = NULL; const struct loadparm_substitution *lp_sub = loadparm_s3_global_substitution(); connection_struct *conn = handle->conn; struct smb_filename *full_fname = NULL; char *path_name = NULL; - char *temp_name = NULL; - char *final_name = NULL; + const char *temp_name = NULL; + const char *final_name = NULL; struct smb_filename *smb_fname_final = NULL; - const char *base; + const char *base = NULL; char *repository = NULL; int i = 1; off_t file_size; /* space_avail; */ @@ -444,9 +445,11 @@ static int recycle_unlink_internal(vfs_handle_struct *handle, struct recycle_config_data, return true); + frame = talloc_stackframe(); + repository = talloc_sub_full( - NULL, - lp_servicename(talloc_tos(), lp_sub, SNUM(conn)), + frame, + lp_servicename(frame, lp_sub, SNUM(conn)), conn->session_info->unix_info->unix_name, conn->connectpath, conn->session_info->unix_token->gid, @@ -468,11 +471,13 @@ static int recycle_unlink_internal(vfs_handle_struct *handle, goto done; } - full_fname = full_path_from_dirfsp_atname(talloc_tos(), + full_fname = full_path_from_dirfsp_atname(frame, dirfsp, smb_fname); if (full_fname == NULL) { - return -1; + rc = -1; + errno = ENOMEM; + goto done; } /* we don't recycle the recycle bin... */ @@ -541,7 +546,7 @@ static int recycle_unlink_internal(vfs_handle_struct *handle, */ /* extract filename and path */ - if (!parent_dirname(talloc_tos(), full_fname->base_name, &path_name, &base)) { + if (!parent_dirname(frame, full_fname->base_name, &path_name, &base)) { rc = -1; errno = ENOMEM; goto done; @@ -573,13 +578,16 @@ static int recycle_unlink_internal(vfs_handle_struct *handle, } if (config->keeptree) { - if (asprintf(&temp_name, "%s/%s", repository, path_name) == -1) { - ALLOC_CHECK(temp_name, done); + temp_name = talloc_asprintf(frame, "%s/%s", + config->repository, + path_name); + if (temp_name == NULL) { + rc = -1; + goto done; } } else { - temp_name = SMB_STRDUP(repository); + temp_name = config->repository; } - ALLOC_CHECK(temp_name, done); exist = recycle_directory_exist(handle, temp_name); if (exist) { @@ -602,12 +610,15 @@ static int recycle_unlink_internal(vfs_handle_struct *handle, } } - if (asprintf(&final_name, "%s/%s", temp_name, base) == -1) { - ALLOC_CHECK(final_name, done); + final_name = talloc_asprintf(frame, "%s/%s", + temp_name, base); + if (final_name == NULL) { + rc = -1; + goto done; } /* Create smb_fname with final base name and orig stream name. */ - smb_fname_final = synthetic_smb_fname(talloc_tos(), + smb_fname_final = synthetic_smb_fname(frame, final_name, full_fname->stream_name, NULL, @@ -679,12 +690,7 @@ static int recycle_unlink_internal(vfs_handle_struct *handle, recycle_do_touch(handle, smb_fname_final, config->touch_mtime); done: - TALLOC_FREE(path_name); - SAFE_FREE(temp_name); - SAFE_FREE(final_name); - TALLOC_FREE(full_fname); - TALLOC_FREE(smb_fname_final); - TALLOC_FREE(repository); + TALLOC_FREE(frame); return rc; } -- 2.34.1 From c60d60ad3651d9961afa39cd781f08e5631b1e44 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Fri, 14 Jun 2024 10:07:02 +0200 Subject: [PATCH 6/9] vfs_recycle: use the correct return in SMB_VFS_HANDLE_GET_DATA() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15659 Signed-off-by: Stefan Metzmacher Reviewed-by: Martin Schwenke Reviewed-by: Noel Power Reviewed-by: Volker Lendecke (cherry picked from commit b38241da3dd73386c4f41a56d95d33d4e1e3d2de) --- source3/modules/vfs_recycle.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source3/modules/vfs_recycle.c b/source3/modules/vfs_recycle.c index 7f54cef4127a..2293185a5aff 100644 --- a/source3/modules/vfs_recycle.c +++ b/source3/modules/vfs_recycle.c @@ -436,14 +436,14 @@ static int recycle_unlink_internal(vfs_handle_struct *handle, off_t file_size; /* space_avail; */ bool exist; int rc = -1; - struct recycle_config_data *config; + struct recycle_config_data *config = NULL; reload_services(NULL, NULL, false); SMB_VFS_HANDLE_GET_DATA(handle, config, struct recycle_config_data, - return true); + return -1); frame = talloc_stackframe(); -- 2.34.1 From 72d412ee36f1c6f4086a96609ab8559b27137fc1 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Fri, 14 Jun 2024 10:07:02 +0200 Subject: [PATCH 7/9] vfs_recycle: fix memory hierarchy If the configuration is reloaded strings and string lists in recycle_config_data could become stale pointers leading to segmentation faults... BUG: https://bugzilla.samba.org/show_bug.cgi?id=15659 Signed-off-by: Stefan Metzmacher Reviewed-by: Martin Schwenke Reviewed-by: Noel Power Reviewed-by: Volker Lendecke (cherry picked from commit 2175856fef17964cef7cf8618b39736168219eec) --- selftest/knownfail.d/samba3.blackbox.recycle | 2 - source3/modules/vfs_recycle.c | 72 +++++++++++++++----- 2 files changed, 55 insertions(+), 19 deletions(-) delete mode 100644 selftest/knownfail.d/samba3.blackbox.recycle diff --git a/selftest/knownfail.d/samba3.blackbox.recycle b/selftest/knownfail.d/samba3.blackbox.recycle deleted file mode 100644 index bae7f717e095..000000000000 --- a/selftest/knownfail.d/samba3.blackbox.recycle +++ /dev/null @@ -1,2 +0,0 @@ -^samba3.blackbox.recycle.recycle.fileserver -^samba3.blackbox.recycle.check_panic.fileserver diff --git a/source3/modules/vfs_recycle.c b/source3/modules/vfs_recycle.c index 2293185a5aff..f318e0ab1db0 100644 --- a/source3/modules/vfs_recycle.c +++ b/source3/modules/vfs_recycle.c @@ -58,7 +58,8 @@ static int vfs_recycle_connect(struct vfs_handle_struct *handle, struct recycle_config_data *config = NULL; int ret; int t; - const char *buff; + const char *buff = NULL; + const char **tmplist = NULL; ret = SMB_VFS_NEXT_CONNECT(handle, service, user); if (ret < 0) { @@ -75,10 +76,17 @@ static int vfs_recycle_connect(struct vfs_handle_struct *handle, errno = ENOMEM; return -1; } - config->repository = lp_parm_const_string(SNUM(handle->conn), - "recycle", - "repository", - ".recycle"); + buff = lp_parm_const_string(SNUM(handle->conn), + "recycle", + "repository", + ".recycle"); + config->repository = talloc_strdup(config, buff); + if (config->repository == NULL) { + DBG_ERR("talloc_strdup(%s) failed\n", buff); + TALLOC_FREE(config); + errno = ENOMEM; + return -1; + } config->keeptree = lp_parm_bool(SNUM(handle->conn), "recycle", "keeptree", @@ -95,18 +103,48 @@ static int vfs_recycle_connect(struct vfs_handle_struct *handle, "recycle", "touch_mtime", False); - config->exclude = lp_parm_string_list(SNUM(handle->conn), - "recycle", - "exclude", - NULL); - config->exclude_dir = lp_parm_string_list(SNUM(handle->conn), - "recycle", - "exclude_dir", - NULL); - config->noversions = lp_parm_string_list(SNUM(handle->conn), - "recycle", - "noversions", - NULL); + tmplist = lp_parm_string_list(SNUM(handle->conn), + "recycle", + "exclude", + NULL); + if (tmplist != NULL) { + char **tmpcpy = str_list_copy(config, tmplist); + if (tmpcpy == NULL) { + DBG_ERR("str_list_copy() failed\n"); + TALLOC_FREE(config); + errno = ENOMEM; + return -1; + } + config->exclude = discard_const_p(const char *, tmpcpy); + } + tmplist = lp_parm_string_list(SNUM(handle->conn), + "recycle", + "exclude_dir", + NULL); + if (tmplist != NULL) { + char **tmpcpy = str_list_copy(config, tmplist); + if (tmpcpy == NULL) { + DBG_ERR("str_list_copy() failed\n"); + TALLOC_FREE(config); + errno = ENOMEM; + return -1; + } + config->exclude_dir = discard_const_p(const char *, tmpcpy); + } + tmplist = lp_parm_string_list(SNUM(handle->conn), + "recycle", + "noversions", + NULL); + if (tmplist != NULL) { + char **tmpcpy = str_list_copy(config, tmplist); + if (tmpcpy == NULL) { + DBG_ERR("str_list_copy() failed\n"); + TALLOC_FREE(config); + errno = ENOMEM; + return -1; + } + config->noversions = discard_const_p(const char *, tmpcpy); + } config->minsize = conv_str_size(lp_parm_const_string( SNUM(handle->conn), "recycle", "minsize", NULL)); config->maxsize = conv_str_size(lp_parm_const_string( -- 2.34.1 From d35a75796ac39e527e8be9ecf1fc7da1e02c8746 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Tue, 18 Jun 2024 14:18:17 +0200 Subject: [PATCH 8/9] Revert "TMP-REPRODUCE: vfs_recycle: demonstrate memory corruption in recycle_unlink_internal()" This was only added to demonstrate the problem more reliable. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15659 Signed-off-by: Stefan Metzmacher Reviewed-by: Martin Schwenke Reviewed-by: Noel Power Reviewed-by: Volker Lendecke (cherry picked from commit c229a84b449b8ba326ee0f6f702d91f101b99ee4) --- source3/modules/vfs_recycle.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/source3/modules/vfs_recycle.c b/source3/modules/vfs_recycle.c index f318e0ab1db0..7827dcd638d7 100644 --- a/source3/modules/vfs_recycle.c +++ b/source3/modules/vfs_recycle.c @@ -476,8 +476,6 @@ static int recycle_unlink_internal(vfs_handle_struct *handle, int rc = -1; struct recycle_config_data *config = NULL; - reload_services(NULL, NULL, false); - SMB_VFS_HANDLE_GET_DATA(handle, config, struct recycle_config_data, -- 2.34.1 From 7168905052f7a6d4c4cf0b7a57cf281a455fe14e Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Fri, 14 Jun 2024 10:07:02 +0200 Subject: [PATCH 9/9] vfs_recycle: remember resolved config->repository in vfs_recycle_connect() This should not change during the lifetime of the tcon. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15659 Signed-off-by: Stefan Metzmacher Reviewed-by: Martin Schwenke Reviewed-by: Noel Power Reviewed-by: Volker Lendecke (cherry picked from commit 53b72ea4d25d4aa6cf8de1c7555456d4cc03b809) --- source3/modules/vfs_recycle.c | 46 +++++++++++++++++------------------ 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/source3/modules/vfs_recycle.c b/source3/modules/vfs_recycle.c index 7827dcd638d7..ea0417d96498 100644 --- a/source3/modules/vfs_recycle.c +++ b/source3/modules/vfs_recycle.c @@ -55,11 +55,14 @@ static int vfs_recycle_connect(struct vfs_handle_struct *handle, const char *service, const char *user) { + const struct loadparm_substitution *lp_sub = + loadparm_s3_global_substitution(); struct recycle_config_data *config = NULL; int ret; int t; const char *buff = NULL; const char **tmplist = NULL; + char *repository = NULL; ret = SMB_VFS_NEXT_CONNECT(handle, service, user); if (ret < 0) { @@ -80,13 +83,26 @@ static int vfs_recycle_connect(struct vfs_handle_struct *handle, "recycle", "repository", ".recycle"); - config->repository = talloc_strdup(config, buff); - if (config->repository == NULL) { - DBG_ERR("talloc_strdup(%s) failed\n", buff); + repository = talloc_sub_full( + config, + lp_servicename(talloc_tos(), lp_sub, SNUM(handle->conn)), + handle->conn->session_info->unix_info->unix_name, + handle->conn->connectpath, + handle->conn->session_info->unix_token->gid, + handle->conn->session_info->unix_info->sanitized_username, + handle->conn->session_info->info->domain_name, + buff); + if (repository == NULL) { + DBG_ERR("talloc_sub_full() failed\n"); TALLOC_FREE(config); errno = ENOMEM; return -1; } + /* shouldn't we allow absolute path names here? --metze */ + /* Yes :-). JRA. */ + trim_char(repository, '\0', '/'); + config->repository = repository; + config->keeptree = lp_parm_bool(SNUM(handle->conn), "recycle", "keeptree", @@ -460,16 +476,12 @@ static int recycle_unlink_internal(vfs_handle_struct *handle, int flags) { TALLOC_CTX *frame = NULL; - const struct loadparm_substitution *lp_sub = - loadparm_s3_global_substitution(); - connection_struct *conn = handle->conn; struct smb_filename *full_fname = NULL; char *path_name = NULL; const char *temp_name = NULL; const char *final_name = NULL; struct smb_filename *smb_fname_final = NULL; const char *base = NULL; - char *repository = NULL; int i = 1; off_t file_size; /* space_avail; */ bool exist; @@ -483,21 +495,7 @@ static int recycle_unlink_internal(vfs_handle_struct *handle, frame = talloc_stackframe(); - repository = talloc_sub_full( - frame, - lp_servicename(frame, lp_sub, SNUM(conn)), - conn->session_info->unix_info->unix_name, - conn->connectpath, - conn->session_info->unix_token->gid, - conn->session_info->unix_info->sanitized_username, - conn->session_info->info->domain_name, - config->repository); - ALLOC_CHECK(repository, done); - /* shouldn't we allow absolute path names here? --metze */ - /* Yes :-). JRA. */ - trim_char(repository, '\0', '/'); - - if(!repository || *(repository) == '\0') { + if (config->repository[0] == '\0') { DEBUG(3, ("recycle: repository path not set, purging %s...\n", smb_fname_str_dbg(smb_fname))); rc = SMB_VFS_NEXT_UNLINKAT(handle, @@ -517,8 +515,8 @@ static int recycle_unlink_internal(vfs_handle_struct *handle, } /* we don't recycle the recycle bin... */ - if (strncmp(full_fname->base_name, repository, - strlen(repository)) == 0) { + if (strncmp(full_fname->base_name, config->repository, + strlen(config->repository)) == 0) { DEBUG(3, ("recycle: File is within recycling bin, unlinking ...\n")); rc = SMB_VFS_NEXT_UNLINKAT(handle, dirfsp, -- 2.34.1