From 3af3db5ba49e3fd833fd26305ba91875e844e6f7 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 (cherry picked 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 4f1a667..8dd2695 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -223,6 +223,7 @@ for env in ["fileserver"]: plantestsuite("samba3.blackbox.inherit_owner.default(%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, 'tmp', '0', '0', '-m', 'NT1']) plantestsuite("samba3.blackbox.inherit_owner.full (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, 'inherit_owner', '1', '1', '-m', 'NT1']) plantestsuite("samba3.blackbox.inherit_owner.unix (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, 'inherit_owner_u', '0', '1', '-m', 'NT1']) + 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 a105d1ff470b530024cf0afd87df04c2d46cfdab 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 a5f078eea918bedd48d661b51bd0d196f3ef28e4 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 4368bf5259c54ecb8ed73044c45d91df829dbe06 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