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.

Autobuild-User: Jeremy Allison <jra@samba.org>
Autobuild-Date: Sat Apr  9 02:05:15 CEST 2011 on sn-devel-104

1 files changed, 48 insertions, 51 deletions

diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
index 6e11ffc275..ea41fbb8e9 100644
--- a/source3/modules/vfs_acl_common.c
+++ b/source3/modules/vfs_acl_common.c
@@ -484,14 +484,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)) {
@@ -504,15 +501,31 @@ static NTSTATUS check_parent_acl_common(vfs_handle_struct *handle,
 					(SECINFO_OWNER |
 					 SECINFO_GROUP |
 					 SECINFO_DACL),
-					&parent_desc);
+					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) {
@@ -536,11 +549,6 @@ static NTSTATUS check_parent_acl_common(vfs_handle_struct *handle,
 	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.
 *********************************************************************/
@@ -553,7 +561,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;
@@ -599,29 +606,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;
 		}
 	}
@@ -648,30 +654,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);
@@ -916,6 +905,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,
@@ -960,13 +950,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;
@@ -984,8 +980,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;