Attachment 6396 Details for Bug 8072 – git-am fix for 3.5.next

[patch] git-am fix for 3.5.next

0001-Fix-bug-8072-PANIC-create_file_acl_common-frees-hand.patch (text/plain), 6.82 KB, created by Jeremy Allison on 2011-04-08 22:27:14 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeremy Allison

Created: 2011-04-08 22:27:14 UTC

Size: 6.82 KB

patch

obsolete

>From c49179c9679d923ababaa3d7b6cd5f6b2cfc81c3 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Fri, 8 Apr 2011 15:25:18 -0700
>Subject: [PATCH] Fix bug 8072 - PANIC: create_file_acl_common frees handle two times.
>
>Caused by premature optimisation storing the parent ACL on the
>module handle instead of (correctly) on the file fsp. Previous
>code wasn't reentrant safe. This is less optimal but doesn't
>crash in the specific case :-).
>
>Jeremy.
>---
> source3/modules/vfs_acl_common.c |  111 ++++++++++++++++++-------------------
> 1 files changed, 54 insertions(+), 57 deletions(-)
>
>diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
>index f1884f5..a5ea505 100644
>--- a/source3/modules/vfs_acl_common.c
>+++ b/source3/modules/vfs_acl_common.c
>@@ -479,14 +479,11 @@ static NTSTATUS inherit_new_acl(vfs_handle_struct *handle,
> 				psd);
> }
> 
>-static NTSTATUS check_parent_acl_common(vfs_handle_struct *handle,
>+static NTSTATUS get_parent_acl_common(vfs_handle_struct *handle,
> 				const char *path,
>-				uint32_t access_mask,
> 				struct security_descriptor **pp_parent_desc)
> {
> 	char *parent_name = NULL;
>-	struct security_descriptor *parent_desc = NULL;
>-	uint32_t access_granted = 0;
> 	NTSTATUS status;
> 
> 	if (!parent_dirname(talloc_tos(), path, &parent_name, NULL)) {
>@@ -496,20 +493,39 @@ static NTSTATUS check_parent_acl_common(vfs_handle_struct *handle,
> 	status = get_nt_acl_internal(handle,
> 					NULL,
> 					parent_name,
>-					(OWNER_SECURITY_INFORMATION |
>-					 GROUP_SECURITY_INFORMATION |
>-					 DACL_SECURITY_INFORMATION),
>-					&parent_desc);
>+					(SECINFO_OWNER |
>+					 SECINFO_GROUP |
>+					 SECINFO_DACL),
>+					pp_parent_desc);
> 
> 	if (!NT_STATUS_IS_OK(status)) {
>-		DEBUG(10,("check_parent_acl_common: get_nt_acl_internal "
>+		DEBUG(10,("get_parent_acl_common: get_nt_acl_internal "
> 			"on directory %s for "
> 			"path %s returned %s\n",
> 			parent_name,
> 			path,
> 			nt_errstr(status) ));
>+	}
>+	return status;
>+}
>+
>+static NTSTATUS check_parent_acl_common(vfs_handle_struct *handle,
>+				const char *path,
>+				uint32_t access_mask,
>+				struct security_descriptor **pp_parent_desc)
>+{
>+	char *parent_name = NULL;
>+	struct security_descriptor *parent_desc = NULL;
>+	uint32_t access_granted = 0;
>+	NTSTATUS status;
>+
>+	status = get_parent_acl_common(handle, path, &parent_desc);
>+	if (!NT_STATUS_IS_OK(status)) {
> 		return status;
> 	}
>+	if (pp_parent_desc) {
>+		*pp_parent_desc = parent_desc;
>+	}
> 	status = smb1_file_se_access_check(handle->conn,
> 					parent_desc,
> 					handle->conn->server_info->ptok,
>@@ -525,17 +541,9 @@ static NTSTATUS check_parent_acl_common(vfs_handle_struct *handle,
> 			nt_errstr(status) ));
> 		return status;
> 	}
>-	if (pp_parent_desc) {
>-		*pp_parent_desc = parent_desc;
>-	}
> 	return NT_STATUS_OK;
> }
> 
>-static void free_sd_common(void **ptr)
>-{
>-	TALLOC_FREE(*ptr);
>-}
>-
> /*********************************************************************
>  Check ACL on open. For new files inherit from parent directory.
> *********************************************************************/
>@@ -548,7 +556,6 @@ static int open_acl_common(vfs_handle_struct *handle,
> {
> 	uint32_t access_granted = 0;
> 	struct security_descriptor *pdesc = NULL;
>-	struct security_descriptor *parent_desc = NULL;
> 	bool file_existed = true;
> 	char *fname = NULL;
> 	NTSTATUS status;
>@@ -594,29 +601,28 @@ static int open_acl_common(vfs_handle_struct *handle,
> 		 * Check the parent directory ACL will allow this.
> 		 */
> 		if (flags & O_CREAT) {
>-			struct security_descriptor *psd = NULL;
>+			struct security_descriptor *parent_desc = NULL;
>+			struct security_descriptor **pp_psd = NULL;
> 
> 			status = check_parent_acl_common(handle, fname,
> 					SEC_DIR_ADD_FILE, &parent_desc);
> 			if (!NT_STATUS_IS_OK(status)) {
> 				goto err;
> 			}
>-			/* Cache the parent security descriptor for
>-			 * later use. We do have an fsp here, but to
>-			 * keep the code consistent with the directory
>-			 * case which doesn't, use the handle. */
> 
>-			/* Attach this to the conn, move from talloc_tos(). */
>-			psd = (struct security_descriptor *)talloc_move(handle->conn,
>-				&parent_desc);
>+			/* Cache the parent security descriptor for
>+			 * later use. */
> 
>-			if (!psd) {
>+			pp_psd = VFS_ADD_FSP_EXTENSION(handle,
>+					fsp,
>+					struct security_descriptor *,
>+					NULL);
>+			if (!pp_psd) {
> 				status = NT_STATUS_NO_MEMORY;
> 				goto err;
> 			}
>-			status = NT_STATUS_NO_MEMORY;
>-			SMB_VFS_HANDLE_SET_DATA(handle, psd, free_sd_common,
>-				struct security_descriptor *, goto err);
>+
>+			*pp_psd = parent_desc;
> 			status = NT_STATUS_OK;
> 		}
> 	}
>@@ -643,30 +649,13 @@ static int mkdir_acl_common(vfs_handle_struct *handle, const char *path, mode_t
> 
> 	ret = vfs_stat_smb_fname(handle->conn, path, &sbuf);
> 	if (ret == -1 && errno == ENOENT) {
>-		struct security_descriptor *parent_desc = NULL;
>-		struct security_descriptor *psd = NULL;
>-
> 		/* We're creating a new directory. */
> 		status = check_parent_acl_common(handle, path,
>-				SEC_DIR_ADD_SUBDIR, &parent_desc);
>+				SEC_DIR_ADD_SUBDIR, NULL);
> 		if (!NT_STATUS_IS_OK(status)) {
> 			errno = map_errno_from_nt_status(status);
> 			return -1;
> 		}
>-
>-		/* Cache the parent security descriptor for
>-		 * later use. We don't have an fsp here so
>-		 * use the handle. */
>-
>-		/* Attach this to the conn, move from talloc_tos(). */
>-		psd = (struct security_descriptor *)talloc_move(handle->conn,
>-				&parent_desc);
>-
>-		if (!psd) {
>-			return -1;
>-		}
>-		SMB_VFS_HANDLE_SET_DATA(handle, psd, free_sd_common,
>-			struct security_descriptor *, return -1);
> 	}
> 
> 	return SMB_VFS_NEXT_MKDIR(handle, path, mode);
>@@ -909,6 +898,7 @@ static NTSTATUS create_file_acl_common(struct vfs_handle_struct *handle,
> 	files_struct *fsp = NULL;
> 	int info;
> 	struct security_descriptor *parent_sd = NULL;
>+	struct security_descriptor **pp_parent_sd = NULL;
> 
> 	status = SMB_VFS_NEXT_CREATE_FILE(handle,
> 					req,
>@@ -952,13 +942,19 @@ static NTSTATUS create_file_acl_common(struct vfs_handle_struct *handle,
> 		goto out;
> 	}
> 
>-
>-	/* We must have a cached parent sd in this case.
>-	 * attached to the handle. */
>-
>-	SMB_VFS_HANDLE_GET_DATA(handle, parent_sd,
>-		struct security_descriptor,
>-		goto err);
>+	/* See if we have a cached parent sd, if so, use it. */
>+	pp_parent_sd = (struct security_descriptor **)VFS_FETCH_FSP_EXTENSION(handle, fsp);
>+	if (!pp_parent_sd) {
>+		/* Must be a directory, fetch again (sigh). */
>+		status = get_parent_acl_common(handle,
>+				fsp->fsp_name->base_name,
>+				&parent_sd);
>+		if (!NT_STATUS_IS_OK(status)) {
>+			goto out;
>+		}
>+	} else {
>+		parent_sd = *pp_parent_sd;
>+	}
> 
> 	if (!parent_sd) {
> 		goto err;
>@@ -976,8 +972,9 @@ static NTSTATUS create_file_acl_common(struct vfs_handle_struct *handle,
> 
>   out:
> 
>-	/* Ensure we never leave attached data around. */
>-	SMB_VFS_HANDLE_FREE_DATA(handle);
>+	if (fsp) {
>+		VFS_REMOVE_FSP_EXTENSION(handle, fsp);
>+	}
> 
> 	if (NT_STATUS_IS_OK(status) && pinfo) {
> 		*pinfo = info;
>-- 
>1.7.3.1
>

Flags:

metze: review+

Actions: View

Attachments on bug 8072: 6393 | 6394 | 6396