Fix cli_splice() fallback code when splicing less than a complete file.
Jeremy Allison
jra at samba.org
Thu Jul 12 20:08:19 UTC 2018
Bailey Berro <baileyberro at google.com> found bug:
https://bugzilla.samba.org/show_bug.cgi?id=13527
where the fallback code for cli_splice() is always
asking for SPLICE_BLOCK_SIZE even when the
remaining bytes we need were smaller than that. This works
when using cli_splice() on a complete file, as the cli_read()
terminated the read at the right place.
Includes regression test and fix. Passes local
make test, and is in gitlab-CI:
https://gitlab.com/samba-team/devel/samba/pipelines/25667214
Please review and push if happy !
Cheers,
Jeremy.
-------------- next part --------------
From 27f025f3be9d7fa292661b7dcc8c8a7ec4b6af25 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Thu, 12 Jul 2018 12:15:12 -0700
Subject: [PATCH 1/2] s3: torture: Test SMB1 cli_splice() fallback path when
doing a non-full file splice.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13527
Signed-off-by: Jeremy Allison <jra at samba.org>
---
selftest/knownfail | 2 +
source3/selftest/tests.py | 5 +-
source3/torture/torture.c | 153 ++++++++++++++++++++++++++++++++++++++
3 files changed, 159 insertions(+), 1 deletion(-)
diff --git a/selftest/knownfail b/selftest/knownfail
index 21ef797ec0f..6d7d962baf0 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -16,6 +16,8 @@
^samba3.*rap.sam.*.useradd # Not provided by Samba 3
^samba3.*rap.sam.*.userdelete # Not provided by Samba 3
^samba3.libsmbclient.opendir # This requires a workgroup called 'WORKGROUP' and for netbios browse lists to have been registered
+^samba3.smbtorture_s3.plain\(fileserver\).CLI_SPLICE
+^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).CLI_SPLICE
# see bug 8412
^samba3.smb2.rename.*.simple_nodelete
^samba3.smb2.rename.*.no_share_delete_no_delete_access
diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index c5356d83244..a061483c5f5 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -73,7 +73,7 @@ tests = ["FDPASS", "LOCK1", "LOCK2", "LOCK3", "LOCK4", "LOCK5", "LOCK6", "LOCK7"
"DIR", "DIR1", "DIR-CREATETIME", "TCON", "TCONDEV", "RW1", "RW2", "RW3", "LARGE_READX", "RW-SIGNING",
"OPEN", "XCOPY", "RENAME", "DELETE", "DELETE-LN", "WILDDELETE", "PROPERTIES", "W2K",
"TCON2", "IOCTL", "CHKPATH", "FDSESS", "CHAIN1", "CHAIN2", "OWNER-RIGHTS",
- "CHAIN3", "PIDHIGH",
+ "CHAIN3", "PIDHIGH", "CLI_SPLICE",
"UID-REGRESSION-TEST", "SHORTNAME-TEST",
"CASE-INSENSITIVE-CREATE", "SMB2-BASIC", "NTTRANS-FSCTL", "SMB2-NEGPROT",
"SMB2-SESSION-REAUTH", "SMB2-SESSION-RECONNECT", "SMB2-FTRUNCATE",
@@ -90,6 +90,9 @@ for t in tests:
# this is a negative test to verify that the server rejects
# access without encryption
plantestsuite("samba3.smbtorture_s3.crypt_server(nt4_dc).%s" % t, "nt4_dc", [os.path.join(samba3srcdir, "script/tests/test_smbtorture_s3.sh"), t, '//$SERVER_IP/tmpenc', '$USERNAME', '$PASSWORD', smbtorture3, "", "-l $LOCAL_PATH"])
+ if t == "CLI_SPLICE":
+ # We must test this against the SMB1 fallback.
+ plantestsuite("samba3.smbtorture_s3.plain(fileserver).%s" % t, "fileserver", [os.path.join(samba3srcdir, "script/tests/test_smbtorture_s3.sh"), t, '//$SERVER_IP/tmp', '$USERNAME', '$PASSWORD', smbtorture3, "", "-l $LOCAL_PATH", "-mNT1"])
plantestsuite("samba3.smbtorture_s3.plain(ad_dc_ntvfs).%s" % t, "ad_dc_ntvfs", [os.path.join(samba3srcdir, "script/tests/test_smbtorture_s3.sh"), t, '//$SERVER_IP/tmp', '$USERNAME', '$PASSWORD', smbtorture3, "", "-l $LOCAL_PATH"])
t = "TLDAP"
diff --git a/source3/torture/torture.c b/source3/torture/torture.c
index 412e9b413fa..be763343659 100644
--- a/source3/torture/torture.c
+++ b/source3/torture/torture.c
@@ -44,6 +44,7 @@
#include "lib/util/sys_rw_data.h"
#include "lib/util/base64.h"
#include "lib/util/time.h"
+#include "lib/crypto/md5.h"
extern char *optarg;
extern int optind;
@@ -9240,6 +9241,157 @@ static bool run_cli_echo(int dummy)
return NT_STATUS_IS_OK(status);
}
+static int splice_status(off_t written, void *priv)
+{
+ return true;
+}
+
+static bool run_cli_splice(int dummy)
+{
+ uint8_t *buf = NULL;
+ struct cli_state *cli1 = NULL;
+ bool correct = false;
+ const char *fname_src = "\\splice_src.dat";
+ const char *fname_dst = "\\splice_dst.dat";
+ NTSTATUS status;
+ uint16_t fnum1 = UINT16_MAX;
+ uint16_t fnum2 = UINT16_MAX;
+ size_t file_size = 20*1024*1024;
+ size_t splice_size = 13*1024*0124 + 713;
+ MD5_CTX md5_ctx;
+ uint8_t digest1[16], digest2[16];
+ off_t written = 0;
+ size_t nread = 0;
+ TALLOC_CTX *frame = talloc_stackframe();
+
+ printf("starting cli_splice test\n");
+
+ if (!torture_open_connection(&cli1, 0)) {
+ goto out;
+ }
+
+ cli_unlink(cli1, fname_src,
+ FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN);
+ cli_unlink(cli1, fname_dst,
+ FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN);
+
+ /* Create a file */
+ status = cli_ntcreate(cli1, fname_src, 0, GENERIC_ALL_ACCESS,
+ FILE_ATTRIBUTE_NORMAL, 0, FILE_OVERWRITE_IF,
+ 0, 0, &fnum1, NULL);
+
+ if (!NT_STATUS_IS_OK(status)) {
+ d_printf("open %s failed: %s\n", fname_src, nt_errstr(status));
+ goto out;
+ }
+
+ /* Write file_size bytes - must be bigger than splice_size. */
+ buf = talloc_zero_array(frame, uint8_t, file_size);
+ if (buf == NULL) {
+ d_printf("talloc_fail\n");
+ goto out;
+ }
+
+ /* Fill it with random numbers. */
+ generate_random_buffer(buf, file_size);
+
+ /* MD5 the first 13MB + 713 bytes. */
+ MD5Init(&md5_ctx);
+ MD5Update(&md5_ctx, buf, splice_size);
+ MD5Final(digest1, &md5_ctx);
+
+ status = cli_writeall(cli1,
+ fnum1,
+ 0,
+ buf,
+ 0,
+ file_size,
+ NULL);
+ if (!NT_STATUS_IS_OK(status)) {
+ d_printf("cli_writeall failed: %s\n", nt_errstr(status));
+ goto out;
+ }
+
+ status = cli_ntcreate(cli1, fname_dst, 0, GENERIC_ALL_ACCESS,
+ FILE_ATTRIBUTE_NORMAL, 0, FILE_OVERWRITE_IF,
+ 0, 0, &fnum2, NULL);
+
+ if (!NT_STATUS_IS_OK(status)) {
+ d_printf("open %s failed: %s\n", fname_dst, nt_errstr(status));
+ goto out;
+ }
+
+ /* Now splice 13MB + 713 bytes. */
+ status = cli_splice(cli1,
+ cli1,
+ fnum1,
+ fnum2,
+ splice_size,
+ 0,
+ 0,
+ &written,
+ splice_status,
+ NULL);
+
+ if (!NT_STATUS_IS_OK(status)) {
+ d_printf("cli_splice failed: %s\n", nt_errstr(status));
+ goto out;
+ }
+
+ /* Clear the old buffer. */
+ memset(buf, '\0', file_size);
+
+ /* Read the new file. */
+ status = cli_read(cli1, fnum2, (char *)buf, 0, splice_size, &nread);
+ if (!NT_STATUS_IS_OK(status)) {
+ d_printf("cli_read failed: %s\n", nt_errstr(status));
+ goto out;
+ }
+ if (nread != splice_size) {
+ d_printf("bad read of 0x%x, should be 0x%x\n",
+ (unsigned int)nread,
+ (unsigned int)splice_size);
+ goto out;
+ }
+
+ /* MD5 the first 13MB + 713 bytes. */
+ MD5Init(&md5_ctx);
+ MD5Update(&md5_ctx, buf, splice_size);
+ MD5Final(digest2, &md5_ctx);
+
+ /* Must be the same. */
+ if (memcmp(digest1, digest2, 16) != 0) {
+ d_printf("bad MD5 compare\n");
+ goto out;
+ }
+
+ correct = true;
+ printf("Success on cli_splice test\n");
+
+ out:
+
+ if (cli1) {
+ if (fnum1 != UINT16_MAX) {
+ cli_close(cli1, fnum1);
+ }
+ if (fnum2 != UINT16_MAX) {
+ cli_close(cli1, fnum2);
+ }
+
+ cli_unlink(cli1, fname_src,
+ FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN);
+ cli_unlink(cli1, fname_dst,
+ FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN);
+
+ if (!torture_close_connection(cli1)) {
+ correct = false;
+ }
+ }
+
+ TALLOC_FREE(frame);
+ return correct;
+}
+
static bool run_uid_regression_test(int dummy)
{
static struct cli_state *cli;
@@ -11651,6 +11803,7 @@ static struct {
{ "NTTRANS-CREATE", run_nttrans_create, 0},
{ "NTTRANS-FSCTL", run_nttrans_fsctl, 0},
{ "CLI_ECHO", run_cli_echo, 0},
+ { "CLI_SPLICE", run_cli_splice, 0},
{ "TLDAP", run_tldap },
{ "STREAMERROR", run_streamerror },
{ "NOTIFY-BENCH", run_notify_bench },
--
2.18.0.203.gfac676dfb9-goog
From fdbeea5d91329b6380e89094b3016be290caf6d8 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Thu, 12 Jul 2018 12:18:50 -0700
Subject: [PATCH 2/2] s3: libsmbclient: Fix cli_splice() fallback when reading
less than a complete file.
We were always asking for SPLICE_BLOCK_SIZE even when the
remaining bytes we wanted were smaller than that. This works
when using cli_splice() on a complete file, as the cli_read()
terminated the read at the right place. We always have the
space to read SPLICE_BLOCK_SIZE bytes so this isn't an overflow.
Found by Bailey Berro <baileyberro at google.com>
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13527
Signed-off-by: Bailey Berro <baileyberro at google.com>
Reviewed-by: Jeremy Allison <jra at samba.org>
---
selftest/knownfail | 2 --
source3/libsmb/clireadwrite.c | 4 +++-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/selftest/knownfail b/selftest/knownfail
index 6d7d962baf0..21ef797ec0f 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -16,8 +16,6 @@
^samba3.*rap.sam.*.useradd # Not provided by Samba 3
^samba3.*rap.sam.*.userdelete # Not provided by Samba 3
^samba3.libsmbclient.opendir # This requires a workgroup called 'WORKGROUP' and for netbios browse lists to have been registered
-^samba3.smbtorture_s3.plain\(fileserver\).CLI_SPLICE
-^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).CLI_SPLICE
# see bug 8412
^samba3.smb2.rename.*.simple_nodelete
^samba3.smb2.rename.*.no_share_delete_no_delete_access
diff --git a/source3/libsmb/clireadwrite.c b/source3/libsmb/clireadwrite.c
index 00ee09ece89..67870d8c40b 100644
--- a/source3/libsmb/clireadwrite.c
+++ b/source3/libsmb/clireadwrite.c
@@ -1462,8 +1462,10 @@ static NTSTATUS cli_splice_fallback(TALLOC_CTX *frame,
*written = 0;
while (remaining) {
+ size_t to_read = MIN(remaining, SPLICE_BLOCK_SIZE);
+
status = cli_read(srccli, src_fnum,
- (char *)buf, src_offset, SPLICE_BLOCK_SIZE,
+ (char *)buf, src_offset, to_read,
&nread);
if (!NT_STATUS_IS_OK(status)) {
return status;
--
2.18.0.203.gfac676dfb9-goog
More information about the samba-technical
mailing list