5494 libndmp memory leaks when setting a property

Reviewed by: Dan Fields <dan.fields@nexenta.com>
Approved by: Gordon Ross <gwr@nexenta.com>

2 files changed, 106 insertions, 107 deletions

diff --git a/usr/src/cmd/ndmpadm/ndmpadm_main.c b/usr/src/cmd/ndmpadm/ndmpadm_main.c
index 4948283cb6..6cc7cbb7bc 100644
--- a/usr/src/cmd/ndmpadm/ndmpadm_main.c
+++ b/usr/src/cmd/ndmpadm/ndmpadm_main.c
@@ -1,6 +1,6 @@
 /*
  * Copyright (c) 2008, 2010, Oracle and/or its affiliates. All rights reserved.
- * Copyright 2014 Nexenta Systems, Inc.  All rights reserved.
+ * Copyright 2015 Nexenta Systems, Inc.  All rights reserved.
  */
 
 /*
@@ -355,7 +355,7 @@ ndmp_set_config_process(char *propname)
 	ret = ndmp_set_prop(propname, propvalue);
 	if (ret != -1) {
 		if (!ndmp_door_status()) {
-			if (ndmp_service_refresh() == -1)
+			if (ndmp_service_refresh() != 0)
 				(void) fprintf(stdout, gettext("Could not "
 				    "refesh property of service ndmpd\n"));
 		}
@@ -631,7 +631,7 @@ ndmp_enable_auth(int argc, char **argv, ndmp_command_t *cur_cmd)
 				continue;
 			}
 			if (!ndmp_door_status() &&
-			    (ndmp_service_refresh()) == -1) {
+			    (ndmp_service_refresh()) != 0) {
 				(void) fprintf(stdout,
 				    gettext("Could not refesh ndmpd service "
 				    "properties\n"));
@@ -692,7 +692,7 @@ ndmp_disable_auth(int argc, char **argv, ndmp_command_t *cur_cmd)
 				continue;
 			}
 			if (!ndmp_door_status() &&
-			    (ndmp_service_refresh()) == -1) {
+			    (ndmp_service_refresh()) != 0) {
 				(void) fprintf(stdout, gettext("Could not "
 				    "refesh ndmpd service properties\n"));
 			}
diff --git a/usr/src/lib/libndmp/common/libndmp_prop.c b/usr/src/lib/libndmp/common/libndmp_prop.c
index f29aadb4a2..27564c5bf5 100644
--- a/usr/src/lib/libndmp/common/libndmp_prop.c
+++ b/usr/src/lib/libndmp/common/libndmp_prop.c
@@ -2,7 +2,7 @@
  * Copyright 2008 Sun Microsystems, Inc.  All rights reserved.
  * Use is subject to license terms.
  * Copyright 2012 Milan Jurik. All rights reserved.
- * Copyright 2014 Nexenta Systems, Inc.  All rights reserved.
+ * Copyright 2015 Nexenta Systems, Inc.  All rights reserved.
  */
 
 /*
@@ -74,11 +74,11 @@ typedef struct ndmp_scfhandle {
 	scf_propertygroup_t *scf_pg;
 } ndmp_scfhandle_t;
 
-static int ndmp_config_saveenv(ndmp_scfhandle_t *);
+static int ndmp_config_saveenv(ndmp_scfhandle_t *, boolean_t);
 static ndmp_scfhandle_t *ndmp_smf_scf_init(const char *);
 static void ndmp_smf_scf_fini(ndmp_scfhandle_t *);
 static int ndmp_smf_start_transaction(ndmp_scfhandle_t *);
-static int ndmp_smf_end_transaction(ndmp_scfhandle_t *);
+static int ndmp_smf_end_transaction(ndmp_scfhandle_t *, boolean_t);
 static int ndmp_smf_set_property(ndmp_scfhandle_t *, const char *,
 		const char *);
 static int ndmp_smf_get_property(ndmp_scfhandle_t *, const char *, char *,
@@ -94,11 +94,11 @@ static int ndmp_smf_get_pg_name(ndmp_scfhandle_t *, const char *, char **);
 int
 ndmp_service_refresh(void)
 {
-	if ((smf_get_state(NDMP_INST)) != NULL)
-		return (smf_refresh_instance(NDMP_INST));
+	int rc = smf_refresh_instance(NDMP_INST);
 
-	ndmp_errno = ENDMP_SMF_INTERNAL;
-	return (-1);
+	if (rc != 0)
+		ndmp_errno = ENDMP_SMF_INTERNAL;
+	return (rc);
 }
 
 /*
@@ -109,26 +109,26 @@ ndmp_service_refresh(void)
 int
 ndmp_get_prop(const char *prop, char **value)
 {
-	ndmp_scfhandle_t *handle = NULL;
-	char *lval = (char *)malloc(NDMP_PROP_LEN);
+	ndmp_scfhandle_t *handle;
+	char *lval;
 	char *pgname;
 
-	if (!lval) {
-		ndmp_errno = ENDMP_MEM_ALLOC;
-		return (-1);
-	}
+	*value = NULL;
 	if ((handle = ndmp_smf_scf_init(NDMP_GROUP_FMRI_PREFIX)) == NULL) {
-		free(lval);
 		return (-1);
 	}
 	if (ndmp_smf_get_pg_name(handle, prop, &pgname)) {
-		free(lval);
+		ndmp_smf_scf_fini(handle);
 		ndmp_errno = ENDMP_SMF_PROP_GRP;
 		return (-1);
 	}
 	if (ndmp_smf_create_service_pgroup(handle, pgname)) {
 		ndmp_smf_scf_fini(handle);
-		free(lval);
+		return (-1);
+	}
+	if ((lval = malloc(NDMP_PROP_LEN)) == NULL) {
+		ndmp_smf_scf_fini(handle);
+		ndmp_errno = ENDMP_MEM_ALLOC;
 		return (-1);
 	}
 	if (ndmp_smf_get_property(handle, prop, lval, NDMP_PROP_LEN) != 0) {
@@ -145,36 +145,38 @@ ndmp_get_prop(const char *prop, char **value)
 int
 ndmp_set_prop(const char *env, const char *env_val)
 {
-	ndmp_scfhandle_t *handle = NULL;
+	ndmp_scfhandle_t *handle;
 	char *pgname;
+	int rc;
 
 	if ((handle = ndmp_smf_scf_init(NDMP_GROUP_FMRI_PREFIX)) == NULL)
 		return (-1);
 
 	if (ndmp_smf_get_pg_name(handle, env, &pgname)) {
+		ndmp_smf_scf_fini(handle);
 		ndmp_errno = ENDMP_SMF_PROP_GRP;
 		return (-1);
 	}
 
-	if (ndmp_smf_create_service_pgroup(handle, pgname))
+	if (ndmp_smf_create_service_pgroup(handle, pgname)) {
+		ndmp_smf_scf_fini(handle);
 		return (-1);
+	}
 
-	if (ndmp_smf_start_transaction(handle))
+	if (ndmp_smf_start_transaction(handle)) {
+		ndmp_smf_scf_fini(handle);
 		return (-1);
-
-	if (env_val) {
-		if (ndmp_smf_set_property(handle, env, env_val)) {
-			return (-1);
-		}
-	} else {
-		if (ndmp_smf_delete_property(handle, env))
-			return (-1);
 	}
 
-	if (ndmp_config_saveenv(handle) != 0)
-		return (-1);
+	if (env_val)
+		rc = ndmp_smf_set_property(handle, env, env_val);
+	else
+		rc = ndmp_smf_delete_property(handle, env);
 
-	return (0);
+	if (ndmp_config_saveenv(handle, (rc == 0)) == 0)
+		return (rc);
+	else
+		return (-1);
 }
 
 static int
@@ -218,11 +220,11 @@ ndmp_smf_get_pg_name(ndmp_scfhandle_t *h, const char *pname, char **pgname)
  * Basically commit the transaction.
  */
 static int
-ndmp_config_saveenv(ndmp_scfhandle_t *handle)
+ndmp_config_saveenv(ndmp_scfhandle_t *handle, boolean_t commit)
 {
 	int ret = 0;
 
-	ret = ndmp_smf_end_transaction(handle);
+	ret = ndmp_smf_end_transaction(handle, commit);
 
 	ndmp_smf_scf_fini(handle);
 	return (ret);
@@ -236,15 +238,16 @@ ndmp_config_saveenv(ndmp_scfhandle_t *handle)
 static void
 ndmp_smf_scf_fini(ndmp_scfhandle_t *handle)
 {
-	if (handle != NULL) {
-		scf_scope_destroy(handle->scf_scope);
-		scf_service_destroy(handle->scf_service);
-		scf_pg_destroy(handle->scf_pg);
-		handle->scf_state = NDMP_SCH_STATE_UNINIT;
-		(void) scf_handle_unbind(handle->scf_handle);
-		scf_handle_destroy(handle->scf_handle);
-		free(handle);
-	}
+	if (handle == NULL)
+		return;
+
+	scf_scope_destroy(handle->scf_scope);
+	scf_service_destroy(handle->scf_service);
+	scf_pg_destroy(handle->scf_pg);
+	handle->scf_state = NDMP_SCH_STATE_UNINIT;
+	(void) scf_handle_unbind(handle->scf_handle);
+	scf_handle_destroy(handle->scf_handle);
+	free(handle);
 }
 
 /*
@@ -381,18 +384,22 @@ ndmp_smf_start_transaction(ndmp_scfhandle_t *handle)
  * necessary cleanup.
  */
 static int
-ndmp_smf_end_transaction(ndmp_scfhandle_t *handle)
+ndmp_smf_end_transaction(ndmp_scfhandle_t *handle, boolean_t commit)
 {
-	if (scf_transaction_commit(handle->scf_trans) < 0) {
-		ndmp_errno = ENDMP_SMF_INTERNAL;
-		return (-1);
+	int rc = 0;
+
+	if (commit) {
+		if (scf_transaction_commit(handle->scf_trans) < 0) {
+			ndmp_errno = ENDMP_SMF_INTERNAL;
+			rc = -1;
+		}
 	}
 
 	scf_transaction_destroy_children(handle->scf_trans);
 	scf_transaction_destroy(handle->scf_trans);
 	handle->scf_trans = NULL;
 
-	return (0);
+	return (rc);
 }
 
 /*
@@ -431,13 +438,13 @@ ndmp_smf_delete_property(ndmp_scfhandle_t *handle, const char *propname)
  * Sets property in current pg
  */
 static int
-ndmp_smf_set_property(ndmp_scfhandle_t *handle,
-    const char *propname, const char *valstr)
+ndmp_smf_set_property(ndmp_scfhandle_t *handle, const char *propname,
+    const char *valstr)
 {
 	int ret = 0;
 	scf_value_t *value = NULL;
 	scf_transaction_entry_t *entry = NULL;
-	scf_property_t *prop;
+	scf_property_t *prop = NULL;
 	scf_type_t type;
 	int64_t valint;
 	uint8_t valbool;
@@ -446,68 +453,60 @@ ndmp_smf_set_property(ndmp_scfhandle_t *handle,
 	 * Properties must be set in transactions and don't take effect until
 	 * the transaction has been ended/committed.
 	 */
-	if (((value = scf_value_create(handle->scf_handle)) != NULL) &&
-	    (entry = scf_entry_create(handle->scf_handle)) != NULL) {
-		if (((prop =
-		    scf_property_create(handle->scf_handle)) != NULL) &&
-		    ((scf_pg_get_property(handle->scf_pg, propname,
-		    prop)) == 0)) {
-			if (scf_property_get_value(prop, value) == 0) {
-				type = scf_value_type(value);
-				if ((scf_transaction_property_change(
-				    handle->scf_trans, entry, propname,
-				    type) == 0) ||
-				    (scf_transaction_property_new(
-				    handle->scf_trans, entry, propname,
-				    type) == 0)) {
-					switch (type) {
-					case SCF_TYPE_ASTRING:
-						if ((scf_value_set_astring(
-						    value,
-						    valstr)) != SCF_SUCCESS)
-							ret = -1;
-						break;
-					case SCF_TYPE_INTEGER:
-						valint = strtoll(valstr, 0, 0);
-						scf_value_set_integer(value,
-						    valint);
-						break;
-					case SCF_TYPE_BOOLEAN:
-						if (strncmp(valstr, "yes", 3))
-							valbool = 0;
-						else
-							valbool = 1;
-						scf_value_set_boolean(value,
-						    valbool);
-						break;
-					default:
-						ret = -1;
-					}
-					if (scf_entry_add_value(entry,
-					    value) != 0) {
-						ret = -1;
-						scf_value_destroy(value);
-					}
-					/* The value is in the transaction */
-					value = NULL;
-				}
-				/* The entry is in the transaction */
-				entry = NULL;
-			} else {
-				ret = -1;
-			}
-		} else {
+	if (((value = scf_value_create(handle->scf_handle)) == NULL) ||
+	    ((entry = scf_entry_create(handle->scf_handle)) == NULL) ||
+	    ((prop = scf_property_create(handle->scf_handle)) == NULL) ||
+	    (scf_pg_get_property(handle->scf_pg, propname, prop) != 0) ||
+	    (scf_property_get_value(prop, value) != 0)) {
+		ret = -1;
+		goto out;
+	}
+
+	type = scf_value_type(value);
+	if ((scf_transaction_property_change(handle->scf_trans, entry, propname,
+	    type) != 0) &&
+	    (scf_transaction_property_new(handle->scf_trans, entry, propname,
+	    type) != 0)) {
+		ret = -1;
+		goto out;
+	}
+
+	switch (type) {
+	case SCF_TYPE_ASTRING:
+		if ((scf_value_set_astring(value, valstr)) != SCF_SUCCESS)
 			ret = -1;
-		}
+		break;
+	case SCF_TYPE_INTEGER:
+		valint = strtoll(valstr, 0, 0);
+		scf_value_set_integer(value, valint);
+		break;
+	case SCF_TYPE_BOOLEAN:
+		if (strncmp(valstr, "yes", 3))
+			valbool = 0;
+		else
+			valbool = 1;
+		scf_value_set_boolean(value, valbool);
+		break;
+	default:
+		ret = -1;
+	}
+	if (scf_entry_add_value(entry, value) == 0) {
+		/* The value is in the transaction */
+		value = NULL;
 	} else {
 		ret = -1;
 	}
+	/* The entry is in the transaction */
+	entry = NULL;
+
+out:
 	if (ret == -1) {
 		if ((scf_error() == SCF_ERROR_PERMISSION_DENIED))
 			ndmp_errno = ENDMP_SMF_PERM;
 		else
 			ndmp_errno = ENDMP_SMF_INTERNAL;
 	}
+	scf_property_destroy(prop);
 	scf_value_destroy(value);
 	scf_entry_destroy(entry);
 	return (ret);
@@ -522,8 +521,8 @@ ndmp_smf_get_property(ndmp_scfhandle_t *handle, const char *propname,
     char *valstr, size_t sz)
 {
 	int ret = 0;
-	scf_value_t *value;
-	scf_property_t *prop;
+	scf_value_t *value = NULL;
+	scf_property_t *prop = NULL;
 	scf_type_t type;
 	int64_t valint;
 	uint8_t valbool;