[PATCH] Another vfs_fruit fix
Jeremy Allison
jra at samba.org
Fri Nov 9 21:33:00 UTC 2018
On Fri, Nov 09, 2018 at 05:00:09PM +0100, Ralph Böhme wrote:
> Hi!
>
> Attached is another vfs_fruit fix.
>
> Problem was, a *Windows* client copied a file with a AFP_AfpInfo stream to
> the server, writing != 60 bytes to the stream. vfs_fruit rejected this, a
> macOS server allows it.
>
> Digging deeper with a torture test I found some strage rules that a macOS
> SMB server applies here. I've now modelled fruit to (hopefully) behave like
> macOS.
>
> Please review&push if happy. Thanks!
LGTM. RB+ and pushed ! Thanks.
Jeremy.
> --
> Ralph Boehme, Samba Team https://samba.org/
> Samba Developer, SerNet GmbH https://sernet.de/en/samba/
> GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
> 59E4 AA1E 9B71 2639 9E46
> From d14d02453a7698fc9afbe7f3f2a951c0bb81d25e Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Tue, 6 Nov 2018 12:24:54 +0100
> Subject: [PATCH 1/3] s4:torture/vfs/fruit: torture writing AFP_AfpInfo stream
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=13677
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> selftest/knownfail.d/samba3.vfs.fruit | 3 +
> source4/torture/vfs/fruit.c | 336 ++++++++++++++++++++++++++
> 2 files changed, 339 insertions(+)
>
> diff --git a/selftest/knownfail.d/samba3.vfs.fruit b/selftest/knownfail.d/samba3.vfs.fruit
> index 6307e2b3404..fe188b33b3d 100644
> --- a/selftest/knownfail.d/samba3.vfs.fruit
> +++ b/selftest/knownfail.d/samba3.vfs.fruit
> @@ -1,2 +1,5 @@
> ^samba3.vfs.fruit streams_depot.OS X AppleDouble file conversion\(nt4_dc\)
> ^samba3.vfs.fruit streams_depot.OS X AppleDouble file conversion without embedded xattr\(nt4_dc\)
> +^samba3.vfs.fruit metadata_netatalk.writing_afpinfo\(nt4_dc\)
> +^samba3.vfs.fruit metadata_stream.writing_afpinfo\(nt4_dc\)
> +^samba3.vfs.fruit streams_depot.writing_afpinfo\(nt4_dc\)
> diff --git a/source4/torture/vfs/fruit.c b/source4/torture/vfs/fruit.c
> index 6bdcae6c877..c532afab729 100644
> --- a/source4/torture/vfs/fruit.c
> +++ b/source4/torture/vfs/fruit.c
> @@ -4502,6 +4502,341 @@ static bool test_invalid_afpinfo(struct torture_context *tctx,
> return ret;
> }
>
> +static bool test_writing_afpinfo(struct torture_context *tctx,
> + struct smb2_tree *tree)
> +{
> + const char *fname = "filtest_invalid_afpinfo";
> + const char *sname = "filtest_invalid_afpinfo" AFPINFO_STREAM;
> + const char *streams_afpinfo[] = {
> + "::$DATA",
> + AFPINFO_STREAM
> + };
> + bool ret = true;
> + static AfpInfo *afpi = NULL;
> + char *buf = NULL;
> + char *afpi_buf = NULL;
> + char *zero_buf = NULL;
> + bool broken_osx = torture_setting_bool(tctx, "broken_osx_45759458", false);
> + off_t min_offset_for_2streams = 16;
> + int i;
> + NTSTATUS status;
> + struct test_sizes {
> + off_t offset;
> + size_t size;
> + bool expected_result;
> + } test_sizes[] = {
> + { 0, 1, false},
> + { 0, 2, false},
> + { 0, 3, true},
> + { 0, 4, true},
> + { 0, 14, true},
> + { 0, 15, true},
> + { 0, 16, true},
> + { 0, 24, true},
> + { 0, 34, true},
> + { 0, 44, true},
> + { 0, 54, true},
> + { 0, 55, true},
> + { 0, 56, true},
> + { 0, 57, true},
> + { 0, 58, true},
> + { 0, 59, true},
> + { 0, 60, true},
> + { 0, 61, true},
> + { 0, 64, true},
> + { 0, 1024, true},
> + { 0, 10064, true},
> +
> + { 1, 1, false},
> + { 1, 2, false},
> + { 1, 3, false},
> + { 1, 4, false},
> + { 1, 14, false},
> + { 1, 15, false},
> + { 1, 16, false},
> + { 1, 24, false},
> + { 1, 34, false},
> + { 1, 44, false},
> + { 1, 54, false},
> + { 1, 55, false},
> + { 1, 56, false},
> + { 1, 57, false},
> + { 1, 58, false},
> + { 1, 59, false},
> + { 1, 60, true},
> + { 1, 61, true},
> + { 1, 1024, true},
> + { 1, 10064, true},
> +
> + { 30, 1, false},
> + { 30, 2, false},
> + { 30, 3, false},
> + { 30, 4, false},
> + { 30, 14, false},
> + { 30, 15, false},
> + { 30, 16, false},
> + { 30, 24, false},
> + { 30, 34, false},
> + { 30, 44, false},
> + { 30, 54, false},
> + { 30, 55, false},
> + { 30, 56, false},
> + { 30, 57, false},
> + { 30, 58, false},
> + { 30, 59, false},
> + { 30, 60, true},
> + { 30, 61, true},
> + { 30, 1024, true},
> + { 30, 10064, true},
> +
> + { 58, 1, false},
> + { 58, 2, false},
> + { 58, 3, false},
> + { 58, 4, false},
> + { 58, 14, false},
> + { 58, 15, false},
> + { 58, 16, false},
> + { 58, 24, false},
> + { 58, 34, false},
> + { 58, 44, false},
> + { 58, 54, false},
> + { 58, 55, false},
> + { 58, 56, false},
> + { 58, 57, false},
> + { 58, 58, false},
> + { 58, 59, false},
> + { 58, 60, true},
> + { 58, 61, true},
> + { 58, 1024, true},
> + { 58, 10064, true},
> +
> + { 59, 1, false},
> + { 59, 2, false},
> + { 59, 3, false},
> + { 59, 4, false},
> + { 59, 14, false},
> + { 59, 15, false},
> + { 59, 16, false},
> + { 59, 24, false},
> + { 59, 34, false},
> + { 59, 44, false},
> + { 59, 54, false},
> + { 59, 55, false},
> + { 59, 56, false},
> + { 59, 57, false},
> + { 59, 58, false},
> + { 59, 59, false},
> + { 59, 60, true},
> + { 59, 61, true},
> + { 59, 1024, true},
> + { 59, 10064, true},
> +
> + { 60, 1, false},
> + { 60, 2, false},
> + { 60, 3, false},
> + { 60, 4, false},
> + { 60, 14, false},
> + { 60, 15, false},
> + { 60, 16, false},
> + { 60, 24, false},
> + { 60, 34, false},
> + { 60, 44, false},
> + { 60, 54, false},
> + { 60, 55, false},
> + { 60, 56, false},
> + { 60, 57, false},
> + { 60, 58, false},
> + { 60, 59, false},
> + { 60, 60, true},
> + { 60, 61, true},
> + { 60, 1024, true},
> + { 60, 10064, true},
> +
> + { 61, 1, false},
> + { 61, 2, false},
> + { 61, 3, false},
> + { 61, 4, false},
> + { 61, 14, false},
> + { 61, 15, false},
> + { 61, 16, false},
> + { 61, 24, false},
> + { 61, 34, false},
> + { 61, 44, false},
> + { 61, 54, false},
> + { 61, 55, false},
> + { 61, 56, false},
> + { 61, 57, false},
> + { 61, 58, false},
> + { 61, 59, false},
> + { 61, 60, true},
> + { 61, 61, true},
> + { 61, 1024, true},
> + { 61, 10064, true},
> +
> + { 10000, 1, false},
> + { 10000, 2, false},
> + { 10000, 3, false},
> + { 10000, 4, false},
> + { 10000, 14, false},
> + { 10000, 15, false},
> + { 10000, 16, false},
> + { 10000, 24, false},
> + { 10000, 34, false},
> + { 10000, 44, false},
> + { 10000, 54, false},
> + { 10000, 55, false},
> + { 10000, 56, false},
> + { 10000, 57, false},
> + { 10000, 58, false},
> + { 10000, 59, false},
> + { 10000, 60, true},
> + { 10000, 61, true},
> + { 10000, 1024, true},
> + { 10000, 10064, true},
> +
> + { -1, 0, false},
> + };
> +
> + afpi = torture_afpinfo_new(tctx);
> + torture_assert_not_null_goto(tctx, afpi, ret, done,
> + "torture_afpinfo_new failed\n");
> +
> + memcpy(afpi->afpi_FinderInfo, "FOO BAR ", 8);
> +
> + buf = torture_afpinfo_pack(afpi, afpi);
> + torture_assert_not_null_goto(tctx, buf, ret, done,
> + "torture_afpinfo_pack failed\n");
> +
> + afpi_buf = talloc_zero_array(tctx, char, 10064);
> + torture_assert_not_null_goto(tctx, afpi_buf, ret, done,
> + "talloc_zero_array failed\n");
> + memcpy(afpi_buf, buf, 60);
> +
> + zero_buf = talloc_zero_array(tctx, char, 10064);
> + torture_assert_not_null_goto(tctx, zero_buf, ret, done,
> + "talloc_zero_array failed\n");
> +
> + ret = torture_setup_file(tctx, tree, fname, false);
> + torture_assert_goto(tctx, ret == true, ret, done,
> + "torture_setup_file\n");
> +
> + for (i = 0; test_sizes[i].offset != -1; i++) {
> + struct smb2_handle h;
> + struct smb2_create c;
> + int expected_num_streams;
> + size_t fi_check_size;
> +
> + torture_comment(tctx,
> + "Test %d: offset=%zd size=%zu result=%s\n",
> + i,
> + test_sizes[i].offset,
> + test_sizes[i].size,
> + test_sizes[i].expected_result ? "true":"false");
> +
> +
> + c = (struct smb2_create) {
> + .in.desired_access = SEC_FILE_WRITE_DATA,
> + .in.file_attributes = FILE_ATTRIBUTE_NORMAL,
> + .in.create_disposition = NTCREATEX_DISP_OPEN_IF,
> + .in.fname = sname,
> + };
> +
> + status = smb2_create(tree, tree, &c);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_create\n");
> + h = c.out.file.handle;
> +
> + status = smb2_util_write(tree,
> + h,
> + zero_buf,
> + test_sizes[i].offset,
> + test_sizes[i].size);
> + torture_assert_ntstatus_equal_goto(
> + tctx, status, NT_STATUS_INVALID_PARAMETER,
> + ret, done, "smb2_util_write\n");
> +
> + status = smb2_util_write(tree,
> + h,
> + afpi_buf,
> + test_sizes[i].offset,
> + test_sizes[i].size);
> + smb2_util_close(tree, h);
> + if (test_sizes[i].expected_result == true) {
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_util_write\n");
> + } else {
> + torture_assert_ntstatus_equal_goto(
> + tctx, status, NT_STATUS_INVALID_PARAMETER,
> + ret, done, "smb2_util_write\n");
> + }
> +
> + if (broken_osx) {
> + /*
> + * Currently macOS has a bug (Radar #45759458) where it
> + * writes more bytes then requested from uninitialized
> + * memory to the filesystem. That means it will likely
> + * write data to FinderInfo so the stream is not empty
> + * and thus listed when the number of streams is
> + * queried.
> + */
> + min_offset_for_2streams = 2;
> + }
> +
> + if ((test_sizes[i].expected_result == true) &&
> + (test_sizes[i].size > min_offset_for_2streams))
> + {
> + expected_num_streams = 2;
> + } else {
> + expected_num_streams = 1;
> + }
> +
> + ret = check_stream_list(tree, tctx, fname,
> + expected_num_streams,
> + streams_afpinfo, false);
> + torture_assert_goto(tctx, ret == true, ret, done,
> + "Bad streams\n");
> +
> + if (test_sizes[i].expected_result == false) {
> + continue;
> + }
> +
> + if (test_sizes[i].size <= 16) {
> + /*
> + * FinderInfo with the "FOO BAR " string we wrote above
> + * would start at offset 16. Check whether this test
> + * wrote 1 byte or more.
> + */
> + goto next;
> + }
> +
> + fi_check_size = test_sizes[i].size - 16;
> + fi_check_size = MIN(fi_check_size, 8);
> +
> + ret = check_stream(tree, __location__,
> + tctx, tctx,
> + fname, AFPINFO_STREAM,
> + 0, 60, 16, fi_check_size, "FOO BAR ");
> + torture_assert_goto(tctx, ret == true, ret, done,
> + "Bad streams\n");
> +
> +next:
> + status = smb2_util_unlink(tree, sname);
> + if (NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_NAME_NOT_FOUND)) {
> + bool missing_ok;
> +
> + missing_ok = test_sizes[i].expected_result == false;
> + missing_ok |= test_sizes[i].size <= 16;
> +
> + torture_assert_goto(tctx, missing_ok,
> + ret, done, "smb2_util_unlink\n");
> + }
> + }
> +
> +done:
> + smb2_util_unlink(tree, fname);
> + return ret;
> +}
> +
> static bool test_zero_file_id(struct torture_context *tctx,
> struct smb2_tree *tree)
> {
> @@ -5913,6 +6248,7 @@ struct torture_suite *torture_vfs_fruit(TALLOC_CTX *ctx)
> torture_suite_add_1smb2_test(suite, "NFS ACE entries", test_nfs_aces);
> torture_suite_add_1smb2_test(suite, "OS X AppleDouble file conversion without embedded xattr", test_adouble_conversion_wo_xattr);
> torture_suite_add_1smb2_test(suite, "empty_stream", test_empty_stream);
> + torture_suite_add_1smb2_test(suite, "writing_afpinfo", test_writing_afpinfo);
>
> return suite;
> }
> --
> 2.17.2
>
>
> From e83ac48feff71e57b805da7fe03478fff375590e Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Tue, 6 Nov 2018 12:34:17 +0100
> Subject: [PATCH 2/3] vfs_fruit: move a comment to the right place
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=13677
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> source3/modules/vfs_fruit.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c
> index efe88efcaeb..59607c8dcf0 100644
> --- a/source3/modules/vfs_fruit.c
> +++ b/source3/modules/vfs_fruit.c
> @@ -4534,6 +4534,12 @@ static ssize_t fruit_pwrite_meta_stream(vfs_handle_struct *handle,
> }
>
> if (ai_empty_finderinfo(ai)) {
> + /*
> + * Writing an all 0 blob to the metadata stream results in the
> + * stream being removed on a macOS server. This ensures we
> + * behave the same and it verified by the "delete AFP_AfpInfo by
> + * writing all 0" test.
> + */
> ret = SMB_VFS_NEXT_FTRUNCATE(handle, fsp, 0);
> if (ret != 0) {
> DBG_ERR("SMB_VFS_NEXT_FTRUNCATE on [%s] failed\n",
> @@ -4606,6 +4612,12 @@ static ssize_t fruit_pwrite_meta_netatalk(vfs_handle_struct *handle,
> return n;
> }
>
> + /*
> + * Writing an all 0 blob to the metadata stream results in the stream
> + * being removed on a macOS server. This ensures we behave the same and
> + * it verified by the "delete AFP_AfpInfo by writing all 0" test.
> + */
> +
> ok = set_delete_on_close(
> fsp,
> true,
> @@ -4627,13 +4639,6 @@ static ssize_t fruit_pwrite_meta(vfs_handle_struct *handle,
> struct fio *fio = (struct fio *)VFS_FETCH_FSP_EXTENSION(handle, fsp);
> ssize_t nwritten;
>
> - /*
> - * Writing an all 0 blob to the metadata stream
> - * results in the stream being removed on a macOS
> - * server. This ensures we behave the same and it
> - * verified by the "delete AFP_AfpInfo by writing all
> - * 0" test.
> - */
> if (n != AFP_INFO_SIZE || offset != 0) {
> DBG_ERR("unexpected offset=%jd or size=%jd\n",
> (intmax_t)offset, (intmax_t)n);
> --
> 2.17.2
>
>
> From f94cedf13a2b9fd3bb2575d16274e552cc62fd5d Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Tue, 6 Nov 2018 13:24:14 +0100
> Subject: [PATCH 3/3] vfs_fruit: validation of writes on AFP_AfpInfo stream
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=13677
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> selftest/knownfail.d/samba3.vfs.fruit | 3 --
> source3/modules/vfs_fruit.c | 67 +++++++++++++++++++++++----
> 2 files changed, 57 insertions(+), 13 deletions(-)
>
> diff --git a/selftest/knownfail.d/samba3.vfs.fruit b/selftest/knownfail.d/samba3.vfs.fruit
> index fe188b33b3d..6307e2b3404 100644
> --- a/selftest/knownfail.d/samba3.vfs.fruit
> +++ b/selftest/knownfail.d/samba3.vfs.fruit
> @@ -1,5 +1,2 @@
> ^samba3.vfs.fruit streams_depot.OS X AppleDouble file conversion\(nt4_dc\)
> ^samba3.vfs.fruit streams_depot.OS X AppleDouble file conversion without embedded xattr\(nt4_dc\)
> -^samba3.vfs.fruit metadata_netatalk.writing_afpinfo\(nt4_dc\)
> -^samba3.vfs.fruit metadata_stream.writing_afpinfo\(nt4_dc\)
> -^samba3.vfs.fruit streams_depot.writing_afpinfo\(nt4_dc\)
> diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c
> index 59607c8dcf0..50b6fac8b95 100644
> --- a/source3/modules/vfs_fruit.c
> +++ b/source3/modules/vfs_fruit.c
> @@ -4638,27 +4638,67 @@ static ssize_t fruit_pwrite_meta(vfs_handle_struct *handle,
> {
> struct fio *fio = (struct fio *)VFS_FETCH_FSP_EXTENSION(handle, fsp);
> ssize_t nwritten;
> + uint8_t buf[AFP_INFO_SIZE];
> + size_t to_write;
> + size_t to_copy;
> + int cmp;
>
> - if (n != AFP_INFO_SIZE || offset != 0) {
> - DBG_ERR("unexpected offset=%jd or size=%jd\n",
> - (intmax_t)offset, (intmax_t)n);
> + if (fio == NULL) {
> + DBG_ERR("Failed to fetch fsp extension");
> return -1;
> }
>
> - if (fio == NULL) {
> - DBG_ERR("Failed to fetch fsp extension");
> + if (n < 3) {
> + errno = EINVAL;
> return -1;
> }
>
> + if (offset != 0 && n < 60) {
> + errno = EINVAL;
> + return -1;
> + }
> +
> + cmp = memcmp(data, "AFP", 3);
> + if (cmp != 0) {
> + errno = EINVAL;
> + return -1;
> + }
> +
> + if (n <= AFP_OFF_FinderInfo) {
> + /*
> + * Nothing to do here really, just return
> + */
> + return n;
> + }
> +
> + offset = 0;
> +
> + to_copy = n;
> + if (to_copy > AFP_INFO_SIZE) {
> + to_copy = AFP_INFO_SIZE;
> + }
> + memcpy(buf, data, to_copy);
> +
> + to_write = n;
> + if (to_write != AFP_INFO_SIZE) {
> + to_write = AFP_INFO_SIZE;
> + }
> +
> switch (fio->config->meta) {
> case FRUIT_META_STREAM:
> - nwritten = fruit_pwrite_meta_stream(handle, fsp, data,
> - n, offset);
> + nwritten = fruit_pwrite_meta_stream(handle,
> + fsp,
> + buf,
> + to_write,
> + offset);
> break;
>
> case FRUIT_META_NETATALK:
> - nwritten = fruit_pwrite_meta_netatalk(handle, fsp, data,
> - n, offset);
> + nwritten = fruit_pwrite_meta_netatalk(handle,
> + fsp,
> + buf,
> + to_write,
> + offset);
> break;
>
> default:
> @@ -4666,7 +4706,14 @@ static ssize_t fruit_pwrite_meta(vfs_handle_struct *handle,
> return -1;
> }
>
> - return nwritten;
> + if (nwritten != to_write) {
> + return -1;
> + }
> +
> + /*
> + * Return the requested amount, verified against macOS SMB server
> + */
> + return n;
> }
>
> static ssize_t fruit_pwrite_rsrc_stream(vfs_handle_struct *handle,
> --
> 2.17.2
>
More information about the samba-technical
mailing list