From e59ceb30e14b102e77e7979993062dd4487d35e6 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Wed, 6 Dec 2017 22:05:23 +0100 Subject: [PATCH 1/5] s4/torture/fruit: ensure AFP_AfpInfo blobs are 0-initialized Bug: https://bugzilla.samba.org/show_bug.cgi?id=13181 Signed-off-by: Ralph Boehme Reviewed-by: Jeremy Allison (cherry picked from commit a22833c2971dc7234b32741305f40ed62e232e0b) --- source4/torture/vfs/fruit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source4/torture/vfs/fruit.c b/source4/torture/vfs/fruit.c index 76890ff75e0..88e3de27059 100644 --- a/source4/torture/vfs/fruit.c +++ b/source4/torture/vfs/fruit.c @@ -909,7 +909,7 @@ static char *torture_afpinfo_pack(TALLOC_CTX *mem_ctx, { char *buf; - buf = talloc_array(mem_ctx, char, AFP_INFO_SIZE); + buf = talloc_zero_array(mem_ctx, char, AFP_INFO_SIZE); if (buf == NULL) { return NULL; } -- 2.13.6 From 475c3e97e19ea1a28b1d82314d40ad6f2b7d20d0 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 7 Dec 2017 13:43:02 +0100 Subject: [PATCH 2/5] s4/torture/fruit: enhance zero AFP_AfpInfo stream test This test more operations in the zeroed out FinderInfo test, ensuring after zeroing out FinderInfo, operations on the filehandle still work and that enumerating streams doesn't return the stream anymore. Bug: https://bugzilla.samba.org/show_bug.cgi?id=13181 Signed-off-by: Ralph Boehme Reviewed-by: Jeremy Allison (backported from commit df31e94eb6241f5e5594f6fd0ec1ad7896e02e27) --- selftest/knownfail.d/samba3.vfs.fruit | 2 + source4/torture/vfs/fruit.c | 87 +++++++++++++++++++++++++++++++++-- 2 files changed, 86 insertions(+), 3 deletions(-) create mode 100644 selftest/knownfail.d/samba3.vfs.fruit diff --git a/selftest/knownfail.d/samba3.vfs.fruit b/selftest/knownfail.d/samba3.vfs.fruit new file mode 100644 index 00000000000..1ba64febb20 --- /dev/null +++ b/selftest/knownfail.d/samba3.vfs.fruit @@ -0,0 +1,2 @@ +^samba3.vfs.fruit metadata_stream.delete AFP_AfpInfo by writing all 0\(nt4_dc\) +^samba3.vfs.fruit streams_depot.delete AFP_AfpInfo by writing all 0\(nt4_dc\) diff --git a/source4/torture/vfs/fruit.c b/source4/torture/vfs/fruit.c index 88e3de27059..05503b01c5e 100644 --- a/source4/torture/vfs/fruit.c +++ b/source4/torture/vfs/fruit.c @@ -3340,11 +3340,17 @@ static bool test_afpinfo_all0(struct torture_context *tctx, { bool ret = true; NTSTATUS status; - struct smb2_handle h1; + struct smb2_create create; + struct smb2_handle h1 = {{0}}; + struct smb2_handle baseh = {{0}}; + union smb_setfileinfo setfinfo; + union smb_fileinfo getfinfo; TALLOC_CTX *mem_ctx = talloc_new(tctx); const char *fname = BASEDIR "\\file"; + const char *sname = BASEDIR "\\file" AFPINFO_STREAM; const char *type_creator = "SMB,OLE!"; AfpInfo *info = NULL; + char *infobuf = NULL; const char *streams_basic[] = { "::$DATA" }; @@ -3375,13 +3381,88 @@ static bool test_afpinfo_all0(struct torture_context *tctx, /* Write all 0 to AFP_AfpInfo */ memset(info->afpi_FinderInfo, 0, AFP_FinderSize); - ret = torture_write_afpinfo(tree, tctx, mem_ctx, fname, info); - torture_assert_goto(tctx, ret == true, ret, done, "torture_write_afpinfo failed"); + infobuf = torture_afpinfo_pack(mem_ctx, info); + torture_assert_not_null_goto(tctx, infobuf, ret, done, + "torture_afpinfo_pack failed\n"); + + ZERO_STRUCT(create); + create.in.desired_access = SEC_FILE_ALL; + create.in.file_attributes = FILE_ATTRIBUTE_NORMAL; + create.in.create_disposition = NTCREATEX_DISP_OPEN; + create.in.share_access = NTCREATEX_SHARE_ACCESS_MASK; + create.in.fname = fname; + + status = smb2_create(tree, mem_ctx, &create); + torture_assert_goto(tctx, ret == true, ret, done, + "smb2_create failed\n"); + baseh = create.out.file.handle; + + ZERO_STRUCT(create); + create.in.desired_access = SEC_FILE_ALL; + create.in.file_attributes = FILE_ATTRIBUTE_NORMAL; + create.in.create_disposition = NTCREATEX_DISP_OVERWRITE_IF; + create.in.fname = sname; + + status = smb2_create(tree, mem_ctx, &create); + torture_assert_goto(tctx, ret == true, ret, done, + "smb2_create failed\n"); + h1 = create.out.file.handle; + + status = smb2_util_write(tree, h1, infobuf, 0, AFP_INFO_SIZE); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "smb2_util_write failed\n"); + + /* + * Get stream information on open handle, must return only default + * stream, the AFP_AfpInfo stream must not be returned. + */ + + ZERO_STRUCT(getfinfo); + getfinfo.generic.level = RAW_FILEINFO_STREAM_INFORMATION; + getfinfo.generic.in.file.handle = baseh; + + status = smb2_getinfo_file(tree, tctx, &getfinfo); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "get stream info\n"); + + torture_assert_int_equal_goto(tctx, getfinfo.stream_info.out.num_streams, + 1, ret, done, "stream count"); + + smb2_util_close(tree, baseh); + ZERO_STRUCT(baseh); + + /* + * Try to set some file-basic-info (time) on the stream. This catches + * naive implementation mistakes that simply deleted the backing store + * from the filesystem in the zero-out step. + */ + + ZERO_STRUCT(setfinfo); + unix_to_nt_time(&setfinfo.basic_info.in.write_time, time(NULL)); + setfinfo.basic_info.in.attrib = 0x20; + setfinfo.generic.level = RAW_SFILEINFO_BASIC_INFORMATION; + setfinfo.generic.in.file.handle = h1; + + status = smb2_setinfo_file(tree, &setfinfo); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "smb2_getinfo_file failed\n"); + + ret = check_stream_list(tree, tctx, fname, 1, streams_basic, false); + torture_assert_goto(tctx, ret == true, ret, done, "check_stream_list"); + + smb2_util_close(tree, h1); + ZERO_STRUCT(h1); ret = check_stream_list(tree, tctx, fname, 1, streams_basic, false); torture_assert_goto(tctx, ret == true, ret, done, "Bad streams"); done: + if (!smb2_util_handle_empty(h1)) { + smb2_util_close(tree, h1); + } + if (!smb2_util_handle_empty(baseh)) { + smb2_util_close(tree, baseh); + } smb2_util_unlink(tree, fname); smb2_util_rmdir(tree, BASEDIR); return ret; -- 2.13.6 From dc39bdd769f3a34e1f120e3c8fcf95dc6ea67eaf Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 7 Dec 2017 14:56:36 +0100 Subject: [PATCH 3/5] vfs_fruit: factor out delete_invalid_meta_stream() from fruit_streaminfo_meta_stream() No change in behaviour, just some refactoring before adding more code to fruit_streaminfo_meta_stream() in the next commit. Bug: https://bugzilla.samba.org/show_bug.cgi?id=13181 Signed-off-by: Ralph Boehme Reviewed-by: Jeremy Allison (backported from commit c41e1ea9247611473d30184efd953c61955ead15) --- source3/modules/vfs_fruit.c | 56 +++++++++++++++++++++++++++------------------ 1 file changed, 34 insertions(+), 22 deletions(-) diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c index f7e57d095c9..92e38fbc8a0 100644 --- a/source3/modules/vfs_fruit.c +++ b/source3/modules/vfs_fruit.c @@ -4717,38 +4717,17 @@ static int fruit_fstat(vfs_handle_struct *handle, files_struct *fsp, return rc; } -static NTSTATUS fruit_streaminfo_meta_stream( +static NTSTATUS delete_invalid_meta_stream( vfs_handle_struct *handle, - struct files_struct *fsp, const struct smb_filename *smb_fname, TALLOC_CTX *mem_ctx, unsigned int *pnum_streams, struct stream_struct **pstreams) { - struct stream_struct *stream = *pstreams; - unsigned int num_streams = *pnum_streams; struct smb_filename *sname = NULL; - int i; int ret; bool ok; - for (i = 0; i < num_streams; i++) { - if (strequal_m(stream[i].name, AFPINFO_STREAM)) { - break; - } - } - - if (i == num_streams) { - return NT_STATUS_OK; - } - - if (stream[i].size == AFP_INFO_SIZE) { - return NT_STATUS_OK; - } - - DBG_ERR("Removing invalid AFPINFO_STREAM size [%zd] from [%s]\n", - stream[i].size, smb_fname_str_dbg(smb_fname)); - ok = del_fruit_stream(mem_ctx, pnum_streams, pstreams, AFPINFO_STREAM); if (!ok) { return NT_STATUS_INTERNAL_ERROR; @@ -4772,6 +4751,39 @@ static NTSTATUS fruit_streaminfo_meta_stream( return NT_STATUS_OK; } +static NTSTATUS fruit_streaminfo_meta_stream( + vfs_handle_struct *handle, + struct files_struct *fsp, + const struct smb_filename *smb_fname, + TALLOC_CTX *mem_ctx, + unsigned int *pnum_streams, + struct stream_struct **pstreams) +{ + struct stream_struct *stream = *pstreams; + unsigned int num_streams = *pnum_streams; + int i; + + for (i = 0; i < num_streams; i++) { + if (strequal_m(stream[i].name, AFPINFO_STREAM)) { + break; + } + } + + if (i == num_streams) { + return NT_STATUS_OK; + } + + if (stream[i].size != AFP_INFO_SIZE) { + DBG_ERR("Removing invalid AFPINFO_STREAM size [%jd] from [%s]\n", + (intmax_t)stream[i].size, smb_fname_str_dbg(smb_fname)); + + return delete_invalid_meta_stream(handle, smb_fname, mem_ctx, + pnum_streams, pstreams); + } + + return NT_STATUS_OK; +} + static NTSTATUS fruit_streaminfo_meta_netatalk( vfs_handle_struct *handle, struct files_struct *fsp, -- 2.13.6 From bfdc4fb962a831fe0e00facd182ed9666dcee2af Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 7 Dec 2017 17:32:35 +0100 Subject: [PATCH 4/5] vfs_fruit: filter out AFP_AfpInfo streams with pending delete-on-close This is in preperation of fixing the implementation of removing the AFP_AfpInfo stream by zeroing the FinderInfo out. We currently remove the stream blob from the underyling filesystem backing store, but that results in certain operations to fail on any still open file-handle. The fix comes in the next commit which will convert to backing store delete operation to a set delete-on-close on the stream. This commit adds filtering on streams that have the delete-on-close set. It is only needed for the fruit:metadata=stream case, as with fruit:metadata=netatalk the filtering is already done in fruit_streaminfo_meta_netatalk(). Bug: https://bugzilla.samba.org/show_bug.cgi?id=13181 Signed-off-by: Ralph Boehme Reviewed-by: Jeremy Allison (cherry picked from commit 70d8f7c5d25f35b58620c2db8f57c7c0758267b3) --- source3/modules/vfs_fruit.c | 72 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 71 insertions(+), 1 deletion(-) diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c index 92e38fbc8a0..18cbd23ba07 100644 --- a/source3/modules/vfs_fruit.c +++ b/source3/modules/vfs_fruit.c @@ -4761,7 +4761,16 @@ static NTSTATUS fruit_streaminfo_meta_stream( { struct stream_struct *stream = *pstreams; unsigned int num_streams = *pnum_streams; + struct smb_filename *sname = NULL; + char *full_name = NULL; + uint32_t name_hash; + struct share_mode_lock *lck = NULL; + struct file_id id = {0}; + bool delete_on_close_set; int i; + int ret; + NTSTATUS status; + bool ok; for (i = 0; i < num_streams; i++) { if (strequal_m(stream[i].name, AFPINFO_STREAM)) { @@ -4781,7 +4790,68 @@ static NTSTATUS fruit_streaminfo_meta_stream( pnum_streams, pstreams); } - return NT_STATUS_OK; + /* + * Now check if there's a delete-on-close pending on the stream. If so, + * hide the stream. This behaviour was verified against a macOS 10.12 + * SMB server. + */ + + sname = synthetic_smb_fname(talloc_tos(), + smb_fname->base_name, + AFPINFO_STREAM_NAME, + NULL, 0); + if (sname == NULL) { + status = NT_STATUS_NO_MEMORY; + goto out; + } + + ret = SMB_VFS_NEXT_STAT(handle, sname); + if (ret != 0) { + status = map_nt_error_from_unix(errno); + goto out; + } + + id = SMB_VFS_NEXT_FILE_ID_CREATE(handle, &sname->st); + + lck = get_existing_share_mode_lock(talloc_tos(), id); + if (lck == NULL) { + status = NT_STATUS_OK; + goto out; + } + + full_name = talloc_asprintf(talloc_tos(), + "%s%s", + sname->base_name, + AFPINFO_STREAM); + if (full_name == NULL) { + status = NT_STATUS_NO_MEMORY; + goto out; + } + + status = file_name_hash(handle->conn, full_name, &name_hash); + if (!NT_STATUS_IS_OK(status)) { + goto out; + } + + delete_on_close_set = is_delete_on_close_set(lck, name_hash); + if (delete_on_close_set) { + ok = del_fruit_stream(mem_ctx, + pnum_streams, + pstreams, + AFPINFO_STREAM); + if (!ok) { + status = NT_STATUS_INTERNAL_ERROR; + goto out; + } + } + + status = NT_STATUS_OK; + +out: + TALLOC_FREE(sname); + TALLOC_FREE(lck); + TALLOC_FREE(full_name); + return status; } static NTSTATUS fruit_streaminfo_meta_netatalk( -- 2.13.6 From 5ccaee33970906e6dae192073584bb51fa1f9d4f Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Wed, 6 Dec 2017 22:09:52 +0100 Subject: [PATCH 5/5] vfs_fruit: set delete-on-close for empty finderinfo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We previously removed the stream from the underlying filesystem stream backing store when the client zeroes out FinderInfo in the AFP_AfpInfo stream, but this causes certain operations to fail (eg stat) when trying to access the stream over any file-handle open on that stream. So instead of deleting, set delete-on-close on the stream. The previous commit already implemented not to list list streams with delete-on-close set which is necessary to implemenent correct macOS semantics for this particular stream. Bug: https://bugzilla.samba.org/show_bug.cgi?id=13181 Signed-off-by: Ralph Boehme Reviewed-by: Jeremy Allison Autobuild-User(master): Ralph Böhme Autobuild-Date(master): Tue Jan 9 17:09:12 CET 2018 on sn-devel-144 (backported from commit e61e9e98e9ff461055daae2fe78f0202f7ed8663) --- selftest/knownfail.d/samba3.vfs.fruit | 2 -- source3/modules/vfs_fruit.c | 60 +++++++++++++++++++++-------------- 2 files changed, 36 insertions(+), 26 deletions(-) delete mode 100644 selftest/knownfail.d/samba3.vfs.fruit diff --git a/selftest/knownfail.d/samba3.vfs.fruit b/selftest/knownfail.d/samba3.vfs.fruit deleted file mode 100644 index 1ba64febb20..00000000000 --- a/selftest/knownfail.d/samba3.vfs.fruit +++ /dev/null @@ -1,2 +0,0 @@ -^samba3.vfs.fruit metadata_stream.delete AFP_AfpInfo by writing all 0\(nt4_dc\) -^samba3.vfs.fruit streams_depot.delete AFP_AfpInfo by writing all 0\(nt4_dc\) diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c index 18cbd23ba07..7ae51ee3f6f 100644 --- a/source3/modules/vfs_fruit.c +++ b/source3/modules/vfs_fruit.c @@ -4065,26 +4065,35 @@ static ssize_t fruit_pwrite_meta_stream(vfs_handle_struct *handle, size_t n, off_t offset) { AfpInfo *ai = NULL; - int ret; + size_t nwritten; + bool ok; ai = afpinfo_unpack(talloc_tos(), data); if (ai == NULL) { return -1; } - if (ai_empty_finderinfo(ai)) { - ret = SMB_VFS_NEXT_UNLINK(handle, fsp->fsp_name); - if (ret != 0 && errno != ENOENT && errno != ENOATTR) { - DBG_ERR("Can't delete metadata for %s: %s\n", - fsp_str_dbg(fsp), strerror(errno)); - TALLOC_FREE(ai); - return -1; - } + nwritten = SMB_VFS_NEXT_PWRITE(handle, fsp, data, n, offset); + if (nwritten != n) { + return -1; + } + if (!ai_empty_finderinfo(ai)) { return n; } - return SMB_VFS_NEXT_PWRITE(handle, fsp, data, n, offset); + ok = set_delete_on_close( + fsp, + true, + handle->conn->session_info->security_token, + handle->conn->session_info->unix_token); + if (!ok) { + DBG_ERR("set_delete_on_close on [%s] failed\n", + fsp_str_dbg(fsp)); + return -1; + } + + return n; } static ssize_t fruit_pwrite_meta_netatalk(vfs_handle_struct *handle, @@ -4095,26 +4104,13 @@ static ssize_t fruit_pwrite_meta_netatalk(vfs_handle_struct *handle, AfpInfo *ai = NULL; char *p = NULL; int ret; + bool ok; ai = afpinfo_unpack(talloc_tos(), data); if (ai == NULL) { return -1; } - if (ai_empty_finderinfo(ai)) { - ret = SMB_VFS_REMOVEXATTR(handle->conn, - fsp->fsp_name->base_name, - AFPINFO_EA_NETATALK); - - if (ret != 0 && errno != ENOENT && errno != ENOATTR) { - DBG_ERR("Can't delete metadata for %s: %s\n", - fsp_str_dbg(fsp), strerror(errno)); - return -1; - } - - return n; - } - ad = ad_fget(talloc_tos(), handle, fsp, ADOUBLE_META); if (ad == NULL) { ad = ad_init(talloc_tos(), handle, ADOUBLE_META); @@ -4139,6 +4135,22 @@ static ssize_t fruit_pwrite_meta_netatalk(vfs_handle_struct *handle, } TALLOC_FREE(ad); + + if (!ai_empty_finderinfo(ai)) { + return n; + } + + ok = set_delete_on_close( + fsp, + true, + handle->conn->session_info->security_token, + handle->conn->session_info->unix_token); + if (!ok) { + DBG_ERR("set_delete_on_close on [%s] failed\n", + fsp_str_dbg(fsp)); + return -1; + } + return n; } -- 2.13.6