From f060239905cd1d0cc5af2a702e51e8cb4da44e8e Mon Sep 17 00:00:00 2001 From: Uri Simchoni Date: Sun, 9 Apr 2017 00:20:40 +0300 Subject: [PATCH 1/4] selftest: test fetching a large ACL from vfs_acl_xattr Add a test that fetches an ACL whose size is larger than 4K. BUG: https://bugzilla.samba.org/show_bug.cgi?id=12737 Signed-off-by: Uri Simchoni Reviewed-by: Jeremy Allison (back-ported from commit 5017dfeef24b8d568e0146c085f3f979d688acf2) --- source3/script/tests/test_large_acl.sh | 59 ++++++++++++++++++++++++++++++++++ source3/selftest/tests.py | 1 + 2 files changed, 60 insertions(+) create mode 100755 source3/script/tests/test_large_acl.sh diff --git a/source3/script/tests/test_large_acl.sh b/source3/script/tests/test_large_acl.sh new file mode 100755 index 0000000..9b6901f --- /dev/null +++ b/source3/script/tests/test_large_acl.sh @@ -0,0 +1,59 @@ +#!/bin/bash +# +# Blackbox test for fetching a large ACL +# + +if [ $# -lt 5 ]; then +cat < /dev/null 2>&1 + rm -rf large_acl > /dev/null +} + +cleanup() +{ + $SMBCLIENT //$SERVER/acl_xattr_ign_sysacl_windows -U $USERNAME%$PASSWORD -c 'rm large_acl' > /dev/null 2>&1 +} + +build_files + +test_large_acl() +{ + #An ACL with 200 entries, ~7K + new_acl=$(seq 1001 1200 | sed -r -e '1 i\D:(A;;0x001f01ff;;;WD)' -e 's/(.*)/(A;;0x001f01ff;;;S-1-5-21-11111111-22222222-33333333-\1)/' | tr -d '\n') + $SMBCACLS //$SERVER/acl_xattr_ign_sysacl_windows -U $USERNAME%$PASSWORD --sddl -S $new_acl large_acl + actual_acl=$($SMBCACLS //$SERVER/acl_xattr_ign_sysacl_windows -U $USERNAME%$PASSWORD --sddl --numeric large_acl 2>/dev/null | sed -rn 's/.*(D:.*)/\1/p' | tr -d '\n') + if [ ! "$new_acl" = "$actual_acl" ] ; then + echo -e "expected:\n$new_acl\nactual:\n$actual_acl\n" + return 1 + fi +} + +failed=0 + +testit "able to retrieve a large ACL if VFS supports it" test_large_acl || failed=`expr $failed + 1` + +cleanup + +exit $failed diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py index d0b67fb..31b7556 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -219,6 +219,7 @@ for env in ["fileserver"]: plantestsuite("samba3.blackbox.netshareenum (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_shareenum.sh"), '$SERVER', '$USERNAME', '$PASSWORD', rpcclient]) plantestsuite("samba3.blackbox.acl_xattr (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_acl_xattr.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls]) plantestsuite("samba3.blackbox.smb2.not_casesensitive (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_smb2_not_casesensitive.sh"), '//$SERVER/tmp', '$SERVER_IP', '$USERNAME', '$PASSWORD', '$LOCAL_PATH', smbclient3]) + plantestsuite("samba3.blackbox.large_acl (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_large_acl.sh"), '$SERVER', '$USERNAME', '$PASSWORD', smbclient3, smbcacls]) # # tar command tests -- 2.9.3 From 9ff6efc1ea122cc12501916ba08a12d9d482630f Mon Sep 17 00:00:00 2001 From: Uri Simchoni Date: Thu, 13 Apr 2017 12:50:47 +0300 Subject: [PATCH 2/4] vfs_xattr_tdb: handle case of zero size. With getxattr(), passing a zero buffer size is a way of obtaining actual xattr size. BUG: https://bugzilla.samba.org/show_bug.cgi?id=12737 Signed-off-by: Uri Simchoni Reviewed-by: Jeremy Allison (cherry picked from commit 4dfa2d6a0972847e3b21ddf05077e50ed72c4ea8) --- source3/modules/vfs_xattr_tdb.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/source3/modules/vfs_xattr_tdb.c b/source3/modules/vfs_xattr_tdb.c index b32fbc1..58acf44 100644 --- a/source3/modules/vfs_xattr_tdb.c +++ b/source3/modules/vfs_xattr_tdb.c @@ -85,6 +85,12 @@ static ssize_t xattr_tdb_getxattr(struct vfs_handle_struct *handle, TALLOC_FREE(frame); return -1; } + + if (size == 0) { + TALLOC_FREE(frame); + return xattr_size; + } + if (blob.length > size) { TALLOC_FREE(frame); errno = ERANGE; @@ -125,6 +131,12 @@ static ssize_t xattr_tdb_fgetxattr(struct vfs_handle_struct *handle, TALLOC_FREE(frame); return -1; } + + if (size == 0) { + TALLOC_FREE(frame); + return xattr_size; + } + if (blob.length > size) { TALLOC_FREE(frame); errno = ERANGE; -- 2.9.3 From 7ee7c60fda00d7973e585ea7ad0cbc1497646400 Mon Sep 17 00:00:00 2001 From: Uri Simchoni Date: Sun, 9 Apr 2017 00:40:44 +0300 Subject: [PATCH 3/4] vfs_acl_xattr: factor out fetching of an extended attribute Pure refactoring - add a function that fetches an extended attribute based on either the file descriptor or the file name. BUG: https://bugzilla.samba.org/show_bug.cgi?id=12737 Signed-off-by: Uri Simchoni Reviewed-by: Jeremy Allison (cherry picked from commit 7b775abd9278ae34110ec87d94a736be7f64884a) --- source3/modules/vfs_acl_xattr.c | 44 +++++++++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 13 deletions(-) diff --git a/source3/modules/vfs_acl_xattr.c b/source3/modules/vfs_acl_xattr.c index e1f90ff..85b9a0d 100644 --- a/source3/modules/vfs_acl_xattr.c +++ b/source3/modules/vfs_acl_xattr.c @@ -37,6 +37,35 @@ Pull a security descriptor into a DATA_BLOB from a xattr. *******************************************************************/ +static ssize_t getxattr_do(vfs_handle_struct *handle, + files_struct *fsp, + const struct smb_filename *smb_fname, + const char *xattr_name, + uint8_t *val, + size_t size) +{ + ssize_t sizeret; + int saved_errno = 0; + + become_root(); + if (fsp && fsp->fh->fd != -1) { + sizeret = SMB_VFS_FGETXATTR(fsp, xattr_name, val, size); + } else { + sizeret = SMB_VFS_GETXATTR(handle->conn, smb_fname->base_name, + XATTR_NTACL_NAME, val, size); + } + if (sizeret == -1) { + saved_errno = errno; + } + unbecome_root(); + + if (saved_errno != 0) { + errno = saved_errno; + } + + return sizeret; +} + static NTSTATUS get_acl_blob(TALLOC_CTX *ctx, vfs_handle_struct *handle, files_struct *fsp, @@ -47,7 +76,6 @@ static NTSTATUS get_acl_blob(TALLOC_CTX *ctx, uint8_t *val = NULL; uint8_t *tmp; ssize_t sizeret; - int saved_errno = 0; ZERO_STRUCTP(pblob); @@ -60,21 +88,11 @@ static NTSTATUS get_acl_blob(TALLOC_CTX *ctx, } val = tmp; - become_root(); - if (fsp && fsp->fh->fd != -1) { - sizeret = SMB_VFS_FGETXATTR(fsp, XATTR_NTACL_NAME, val, size); - } else { - sizeret = SMB_VFS_GETXATTR(handle->conn, smb_fname->base_name, - XATTR_NTACL_NAME, val, size); - } - if (sizeret == -1) { - saved_errno = errno; - } - unbecome_root(); + sizeret = + getxattr_do(handle, fsp, smb_fname, XATTR_NTACL_NAME, val, size); /* Max ACL size is 65536 bytes. */ if (sizeret == -1) { - errno = saved_errno; if ((errno == ERANGE) && (size != 65536)) { /* Too small, try again. */ size = 65536; -- 2.9.3 From e9a93164a658781c191309318a9ce51af2366b0a Mon Sep 17 00:00:00 2001 From: Uri Simchoni Date: Thu, 13 Apr 2017 12:44:58 +0300 Subject: [PATCH 4/4] vfs_acl_xattr: avoid needlessly supplying a large buffer to getxattr() When obtaining the security descriptor via getxattr(), first try optimistically to supply a buffer of 4K, and if that turns out to be too small, determine the correct buffer size. The previous behavior of falling back to a 64K buffer encountered problem with Linux prior to version 3.6, due to pyisical memory fragmentation. With those kernels, as long as the buffer is 8K or smaller, getting the xattr is much less prone to failure due to memory fragmentation. BUG: https://bugzilla.samba.org/show_bug.cgi?id=12737 Signed-off-by: Uri Simchoni Reviewed-by: Jeremy Allison Autobuild-User(master): Jeremy Allison Autobuild-Date(master): Tue Apr 18 04:41:16 CEST 2017 on sn-devel-144 (cherry picked from commit 05d83ccf7a6fecf963fcb980acd50cebfc0c3ea9) --- source3/modules/vfs_acl_xattr.c | 44 ++++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 14 deletions(-) diff --git a/source3/modules/vfs_acl_xattr.c b/source3/modules/vfs_acl_xattr.c index 85b9a0d..421860b 100644 --- a/source3/modules/vfs_acl_xattr.c +++ b/source3/modules/vfs_acl_xattr.c @@ -72,7 +72,7 @@ static NTSTATUS get_acl_blob(TALLOC_CTX *ctx, const struct smb_filename *smb_fname, DATA_BLOB *pblob) { - size_t size = 1024; + size_t size = 4096; uint8_t *val = NULL; uint8_t *tmp; ssize_t sizeret; @@ -91,22 +91,38 @@ static NTSTATUS get_acl_blob(TALLOC_CTX *ctx, sizeret = getxattr_do(handle, fsp, smb_fname, XATTR_NTACL_NAME, val, size); - /* Max ACL size is 65536 bytes. */ - if (sizeret == -1) { - if ((errno == ERANGE) && (size != 65536)) { - /* Too small, try again. */ - size = 65536; - goto again; - } + if (sizeret >= 0) { + pblob->data = val; + pblob->length = sizeret; + return NT_STATUS_OK; + } - /* Real error - exit here. */ - TALLOC_FREE(val); - return map_nt_error_from_unix(errno); + if (errno != ERANGE) { + goto err; } - pblob->data = val; - pblob->length = sizeret; - return NT_STATUS_OK; + /* Too small, try again. */ + sizeret = + getxattr_do(handle, fsp, smb_fname, XATTR_NTACL_NAME, NULL, 0); + if (sizeret < 0) { + goto err; + } + + if (size < sizeret) { + size = sizeret; + } + + if (size > 65536) { + /* Max ACL size is 65536 bytes. */ + errno = ERANGE; + goto err; + } + + goto again; + err: + /* Real error - exit here. */ + TALLOC_FREE(val); + return map_nt_error_from_unix(errno); } /******************************************************************* -- 2.9.3