Always opened $Secure when mounting NTFS volume

Currently, applications that wish to access security descriptors have to
explicitly open the volume's security descriptor index ("$Secure") using
ntfs_open_secure().  Applications are also responsible for closing the
index when done with it.  However, the cleanup function for doing,
ntfs_close_secure(), cannot be called easily by all applications because
it requires a SECURITY_CONTEXT argument, not simply the ntfs_volume.
Some applications therefore have to close the inode and index contexts
manually in order to clean up properly.

This proposal updates libntfs-3g to open $Secure unconditonally as part
of ntfs_mount(), so that applications do not have to worry about it.

This proposal updates libntfs-3g to open $Secure unconditonally as part
of ntfs_mount(), so that applications do not have to worry about it.

ntfs_close_secure() is updated to take in a ntfs_volume for internal use,
and ntfs_destroy_security_context() is now the function to call to free
memory associated with a SECURITY_CONTEXT rather than a ntfs_volume.

Some memory leaks in error paths of ntfs_open_secure() are also fixed.

Signed-off-by: Eric Biggers <ebiggers3@gmail.com>

include/ntfs-3g/security.h

@@ -256,7 +256,9 @@ int ntfs_set_owner_mode(struct SECURITY_CONTEXT *scx,
 le32 ntfs_inherited_id(struct SECURITY_CONTEXT *scx,
 		ntfs_inode *dir_ni, BOOL fordir);
 int ntfs_open_secure(ntfs_volume *vol);
-void ntfs_close_secure(struct SECURITY_CONTEXT *scx);
+void ntfs_close_secure(ntfs_volume *vol);
+
+void ntfs_destroy_security_context(struct SECURITY_CONTEXT *scx);
 
 #if POSIXACLS
 

libntfs-3g/security.c

@@ -4466,55 +4466,75 @@ int ntfs_set_ntfs_attrib(ntfs_inode *ni,
 
 
 /*
- *	Open $Secure once for all
+ *	Open the volume's security descriptor index ($Secure)
+ *
  *	returns zero if it succeeds
- *		non-zero if it fails. This is not an error (on NTFS v1.x)
+ *		non-zero if it fails and the NTFS version is at least v3.x
  */
-
-
 int ntfs_open_secure(ntfs_volume *vol)
 {
 	ntfs_inode *ni;
-	int res;
+	ntfs_index_context *sii;
+	ntfs_index_context *sdh;
 
-	res = -1;
+	if (vol->secure_ni) /* Already open? */
-	vol->secure_ni = (ntfs_inode*)NULL;
+		return 0;
-	vol->secure_xsii = (ntfs_index_context*)NULL;
+
-	vol->secure_xsdh = (ntfs_index_context*)NULL;
+	ni = ntfs_pathname_to_inode(vol, NULL, "$Secure");
-	if (vol->major_ver >= 3) {
+	if (!ni)
-			/* make sure this is a genuine $Secure inode 9 */
+		goto err;
-		ni = ntfs_pathname_to_inode(vol, NULL, "$Secure");
+
-		if (ni && (ni->mft_no == 9)) {
+	/* Verify that $Secure has the expected inode number. */
-			vol->secure_reentry = 0;
+	if (ni->mft_no != FILE_Secure) {
-			vol->secure_xsii = ntfs_index_ctx_get(ni,
+		errno = EINVAL;
-						sii_stream, 4);
+		goto err_close_ni;
-			vol->secure_xsdh = ntfs_index_ctx_get(ni,
-						sdh_stream, 4);
-			if (ni && vol->secure_xsii && vol->secure_xsdh) {
-				vol->secure_ni = ni;
-				res = 0;
-			}
-		}
 	}
-	return (res);
+
+	/* Allocate the needed index contexts. */
+	sii = ntfs_index_ctx_get(ni, sii_stream, 4);
+	if (!sii)
+		goto err_close_ni;
+
+	sdh = ntfs_index_ctx_get(ni, sdh_stream, 4);
+	if (!sdh)
+		goto err_close_sii;
+
+	vol->secure_ni = ni;
+	vol->secure_xsii = sii;
+	vol->secure_xsdh = sdh;
+	return 0;
+
+err_close_sii:
+	ntfs_index_ctx_put(sii);
+err_close_ni:
+	ntfs_inode_close(ni);
+err:
+	/* Failing on NTFS versions before 3.x is expected */
+	if (vol->major_ver < 3)
+		return 0;
+	ntfs_log_perror("error opening $Secure");
+	return -1;
 }
 
 /*
- *		Final cleaning
+ *	Close the volume's security descriptor index ($Secure)
- *	Allocated memory is freed to facilitate the detection of memory leaks
  */
-
+void ntfs_close_secure(ntfs_volume *vol)
-void ntfs_close_secure(struct SECURITY_CONTEXT *scx)
 {
-	ntfs_volume *vol;
-
-	vol = scx->vol;
 	if (vol->secure_ni) {
 		ntfs_index_ctx_put(vol->secure_xsii);
 		ntfs_index_ctx_put(vol->secure_xsdh);
 		ntfs_inode_close(vol->secure_ni);
-		
+		vol->secure_ni = NULL;
 	}
+}
+
+/*
+ *		Destroy a security context
+ *	Allocated memory is freed to facilitate the detection of memory leaks
+ */
+void ntfs_destroy_security_context(struct SECURITY_CONTEXT *scx)
+{
 	ntfs_free_mapping(scx->mapping);
 	free_caches(scx);
 }
@@ -5333,7 +5353,6 @@ struct SECURITY_API *ntfs_initialize_file_security(const char *device,
 				scx->vol->secure_flags = 0;
 					/* accept no mapping and no $Secure */
 				ntfs_build_mapping(scx,(const char*)NULL,TRUE);
-				ntfs_open_secure(vol);
 			} else {
 				if (scapi)
 					free(scapi);
@@ -5365,7 +5384,7 @@ BOOL ntfs_leave_file_security(struct SECURITY_API *scapi)
 	ok = FALSE;
 	if (scapi && (scapi->magic == MAGIC_API) && scapi->security.vol) {
 		vol = scapi->security.vol;
-		ntfs_close_secure(&scapi->security);
+		ntfs_destroy_security_context(&scapi->security);
 		free(scapi);
  		if (!ntfs_umount(vol, 0))
 			ok = TRUE;

libntfs-3g/volume.c

@@ -74,6 +74,7 @@
 #include "cache.h"
 #include "realpath.h"
 #include "misc.h"
+#include "security.h"
 
 const char *ntfs_home = 
 "News, support and information:  http://tuxera.com\n";
@@ -206,6 +207,7 @@ static int __ntfs_volume_release(ntfs_volume *v)
 			ntfs_error_set(&err);
 	}
 
+	ntfs_close_secure(v);
 	ntfs_free_lru_caches(v);
 	free(v->vol_name);
 	free(v->upcase);
@@ -1234,6 +1236,11 @@ ntfs_volume *ntfs_device_mount(struct ntfs_device *dev, ntfs_mount_flags flags)
 		ntfs_log_perror("Failed to close $AttrDef");
 		goto error_exit;
 	}
+
+	/* Open $Secure. */
+	if (ntfs_open_secure(vol))
+		goto error_exit;
+
 	/*
 	 * Check for dirty logfile and hibernated Windows.
 	 * We care only about read-write mounts.

src/lowntfs-3g.c

@@ -3852,7 +3852,7 @@ static void ntfs_close(void)
 				 / ctx->seccache->head.p_reads % 10);
 			}
 		}
-		ntfs_close_secure(&security);
+		ntfs_destroy_security_context(&security);
 	}
         
 	if (ntfs_umount(ctx->vol, FALSE))
@@ -4363,10 +4363,6 @@ int main(int argc, char *argv[])
 #ifdef HAVE_SETXATTR	/* extended attributes interface required */
 	ctx->vol->efs_raw = ctx->efs_raw;
 #endif /* HAVE_SETXATTR */
-		/* JPA open $Secure, (whatever NTFS version !) */
-		/* to initialize security data */
-	if (ntfs_open_secure(ctx->vol) && (ctx->vol->major_ver >= 3))
-		failed_secure = "Could not open file $Secure";
 	if (!ntfs_build_mapping(&ctx->security,ctx->usermap_path,
 		(ctx->vol->secure_flags
 			& ((1 << SECURITY_DEFAULT) | (1 << SECURITY_ACL)))

src/ntfs-3g.c

@@ -3651,7 +3651,7 @@ static void ntfs_close(void)
 			         / ctx->seccache->head.p_reads % 10);
 			}
 		}
-		ntfs_close_secure(&security);
+		ntfs_destroy_security_context(&security);
 	}
 	
 	if (ntfs_umount(ctx->vol, FALSE))
@@ -4168,10 +4168,6 @@ int main(int argc, char *argv[])
 #ifdef HAVE_SETXATTR	/* extended attributes interface required */
 	ctx->vol->efs_raw = ctx->efs_raw;
 #endif /* HAVE_SETXATTR */
-		/* JPA open $Secure, (whatever NTFS version !) */
-		/* to initialize security data */
-	if (ntfs_open_secure(ctx->vol) && (ctx->vol->major_ver >= 3))
-		failed_secure = "Could not open file $Secure";
 	if (!ntfs_build_mapping(&ctx->security,ctx->usermap_path,
 		(ctx->vol->secure_flags
 			& ((1 << SECURITY_DEFAULT) | (1 << SECURITY_ACL)))