[PATCH] Fix a panic in vfs_fruit
Jeremy Allison
jra at samba.org
Mon Aug 20 21:04:55 UTC 2018
On Mon, Aug 20, 2018 at 08:45:59PM +0200, Volker Lendecke via samba-technical wrote:
> Hi!
>
> Review appreciated!
Good catch, thanks ! Pushed with reference to bug:
https://bugzilla.samba.org/show_bug.cgi?id=13584
as we'll need a back-port of this.
Jeremy.
> --
> SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
> phone: +49-551-370000-0, fax: +49-551-370000-9
> AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
> http://www.sernet.de, mailto:kontakt at sernet.de
>
> Meet us at Storage Developer Conference (SDC)
> Santa Clara, CA USA, September 24th-27th 2018
> From 5770c84f66cb346c30e848591a2e8a0c2816d758 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Mon, 6 Aug 2018 14:33:34 +0200
> Subject: [PATCH 1/2] vfs_fruit: Fix a leak of "br_lck"
>
> Fix a panic if fruit_access_check detects a locking conflict.
>
> do_lock() returns a valid br_lck even in case of a locking conflict.
> Not free'ing it leads to a invalid lock order panic later, because
> "br_lck" corresponds to a dbwrap lock on brlock.tdb.
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/modules/vfs_fruit.c | 24 ++++++++++++++++--------
> 1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c
> index 191477c0e1d..9635116e0ba 100644
> --- a/source3/modules/vfs_fruit.c
> +++ b/source3/modules/vfs_fruit.c
> @@ -2386,7 +2386,6 @@ static NTSTATUS fruit_check_access(vfs_handle_struct *handle,
> uint32_t deny_mode)
> {
> NTSTATUS status = NT_STATUS_OK;
> - struct byte_range_lock *br_lck = NULL;
> bool open_for_reading, open_for_writing, deny_read, deny_write;
> off_t off;
> bool have_read = false;
> @@ -2444,6 +2443,8 @@ static NTSTATUS fruit_check_access(vfs_handle_struct *handle,
>
> /* Set locks */
> if ((access_mask & FILE_READ_DATA) && have_read) {
> + struct byte_range_lock *br_lck = NULL;
> +
> off = access_to_netatalk_brl(fork_type, FILE_READ_DATA);
> br_lck = do_lock(
> handle->conn->sconn->msg_ctx, fsp,
> @@ -2451,13 +2452,16 @@ static NTSTATUS fruit_check_access(vfs_handle_struct *handle,
> READ_LOCK, POSIX_LOCK, false,
> &status, NULL);
>
> + TALLOC_FREE(br_lck);
> +
> if (!NT_STATUS_IS_OK(status)) {
> return status;
> }
> - TALLOC_FREE(br_lck);
> }
>
> if ((deny_mode & DENY_READ) && have_read) {
> + struct byte_range_lock *br_lck = NULL;
> +
> off = denymode_to_netatalk_brl(fork_type, DENY_READ);
> br_lck = do_lock(
> handle->conn->sconn->msg_ctx, fsp,
> @@ -2465,10 +2469,11 @@ static NTSTATUS fruit_check_access(vfs_handle_struct *handle,
> READ_LOCK, POSIX_LOCK, false,
> &status, NULL);
>
> + TALLOC_FREE(br_lck);
> +
> if (!NT_STATUS_IS_OK(status)) {
> return status;
> }
> - TALLOC_FREE(br_lck);
> }
> }
>
> @@ -2494,6 +2499,8 @@ static NTSTATUS fruit_check_access(vfs_handle_struct *handle,
>
> /* Set locks */
> if ((access_mask & FILE_WRITE_DATA) && have_read) {
> + struct byte_range_lock *br_lck = NULL;
> +
> off = access_to_netatalk_brl(fork_type, FILE_WRITE_DATA);
> br_lck = do_lock(
> handle->conn->sconn->msg_ctx, fsp,
> @@ -2501,13 +2508,15 @@ static NTSTATUS fruit_check_access(vfs_handle_struct *handle,
> READ_LOCK, POSIX_LOCK, false,
> &status, NULL);
>
> + TALLOC_FREE(br_lck);
> +
> if (!NT_STATUS_IS_OK(status)) {
> return status;
> }
> - TALLOC_FREE(br_lck);
> -
> }
> if ((deny_mode & DENY_WRITE) && have_read) {
> + struct byte_range_lock *br_lck = NULL;
> +
> off = denymode_to_netatalk_brl(fork_type, DENY_WRITE);
> br_lck = do_lock(
> handle->conn->sconn->msg_ctx, fsp,
> @@ -2515,15 +2524,14 @@ static NTSTATUS fruit_check_access(vfs_handle_struct *handle,
> READ_LOCK, POSIX_LOCK, false,
> &status, NULL);
>
> + TALLOC_FREE(br_lck);
> +
> if (!NT_STATUS_IS_OK(status)) {
> return status;
> }
> - TALLOC_FREE(br_lck);
> }
> }
>
> - TALLOC_FREE(br_lck);
> -
> return status;
> }
>
> --
> 2.11.0
>
>
> From ef8f5e7000fdbfdbdaa8103f1f77abf8c35cc00e Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Mon, 6 Aug 2018 14:35:15 +0200
> Subject: [PATCH 2/2] torture: Demonstrate the invalid lock order panic
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source4/torture/vfs/fruit.c | 89 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 89 insertions(+)
>
> diff --git a/source4/torture/vfs/fruit.c b/source4/torture/vfs/fruit.c
> index 25c0668ea5d..a9ae891a23f 100644
> --- a/source4/torture/vfs/fruit.c
> +++ b/source4/torture/vfs/fruit.c
> @@ -4936,6 +4936,93 @@ done:
> return ret;
> }
>
> +static bool test_fruit_locking_conflict(struct torture_context *tctx,
> + struct smb2_tree *tree)
> +{
> + TALLOC_CTX *mem_ctx;
> + struct smb2_create create;
> + struct smb2_handle h;
> + struct smb2_lock lck;
> + struct smb2_lock_element el;
> + const char *fname = BASEDIR "\\locking_conflict.txt";
> + NTSTATUS status;
> + bool ret = false;
> +
> + mem_ctx = talloc_new(tctx);
> + torture_assert_not_null(tctx, mem_ctx, "talloc_new failed");
> +
> + /* clean slate ...*/
> + smb2_util_unlink(tree, fname);
> + smb2_deltree(tree, fname);
> + smb2_deltree(tree, BASEDIR);
> +
> + status = torture_smb2_testdir(tree, BASEDIR, &h);
> + CHECK_STATUS(status, NT_STATUS_OK);
> + smb2_util_close(tree, h);
> +
> + create = (struct smb2_create) {
> + .in.desired_access = SEC_RIGHTS_FILE_READ,
> + .in.file_attributes = FILE_ATTRIBUTE_NORMAL,
> + .in.share_access =
> + NTCREATEX_SHARE_ACCESS_READ|
> + NTCREATEX_SHARE_ACCESS_WRITE,
> + .in.create_disposition = NTCREATEX_DISP_CREATE,
> + .in.impersonation_level = SMB2_IMPERSONATION_ANONYMOUS,
> + .in.fname = fname,
> + };
> +
> + status = smb2_create(tree, mem_ctx, &create);
> + CHECK_STATUS(status, NT_STATUS_OK);
> + h = create.out.file.handle;
> +
> + el = (struct smb2_lock_element) {
> + .offset = 0xfffffffffffffffc,
> + .length = 1,
> + .flags = SMB2_LOCK_FLAG_EXCLUSIVE,
> + };
> + lck = (struct smb2_lock) {
> + .in.lock_count = 1,
> + .in.file.handle = h,
> + .in.locks = &el,
> + };
> +
> + status = smb2_lock(tree, &lck);
> + CHECK_STATUS(status, NT_STATUS_OK);
> +
> + el = (struct smb2_lock_element) {
> + .offset = 0,
> + .length = 0x7fffffffffffffff,
> + .flags = SMB2_LOCK_FLAG_EXCLUSIVE,
> + };
> + status = smb2_lock(tree, &lck);
> + CHECK_STATUS(status, NT_STATUS_OK);
> +
> + create = (struct smb2_create) {
> + .in.desired_access =
> + SEC_RIGHTS_FILE_READ|SEC_RIGHTS_FILE_WRITE,
> + .in.file_attributes = FILE_ATTRIBUTE_NORMAL,
> + .in.share_access = NTCREATEX_SHARE_ACCESS_READ,
> + .in.create_disposition = NTCREATEX_DISP_OPEN,
> + .in.impersonation_level = SMB2_IMPERSONATION_ANONYMOUS,
> + .in.fname = fname,
> + };
> +
> + status = smb2_create(tree, mem_ctx, &create);
> + CHECK_STATUS(status, NT_STATUS_FILE_LOCK_CONFLICT);
> +
> + {
> + struct smb2_close cl = {
> + .level = RAW_CLOSE_SMB2,
> + .in.file.handle = h,
> + };
> + smb2_close(tree, &cl);
> + }
> +
> + ret = true;
> +done:
> + return ret;
> +}
> +
> struct torture_suite *torture_vfs_fruit_netatalk(TALLOC_CTX *ctx)
> {
> struct torture_suite *suite = torture_suite_create(
> @@ -4945,6 +5032,8 @@ struct torture_suite *torture_vfs_fruit_netatalk(TALLOC_CTX *ctx)
>
> torture_suite_add_1smb2_test(suite, "read netatalk metadata", test_read_netatalk_metadata);
> torture_suite_add_1smb2_test(suite, "stream names with locally created xattr", test_stream_names_local);
> + torture_suite_add_1smb2_test(
> + suite, "locking conflict", test_fruit_locking_conflict);
>
> return suite;
> }
> --
> 2.11.0
>
More information about the samba-technical
mailing list