Attachment 3281 Details for Bug 22553 – Fix leaks, NPE,

[patch] Fix leaks, NPE,

22553_univention_policy.diff (text/plain), 13.00 KB, created by Philipp Hahn on 2011-05-17 19:41:55 CEST

(hide)

Description:

Filename:

MIME Type:

Creator: Philipp Hahn

Created: 2011-05-17 19:41:55 CEST

Size: 13.00 KB

patch

obsolete

>Index: tools/univention_policy_result.c
>===================================================================
>--- tools/univention_policy_result.c	(Revision 24237)
>+++ tools/univention_policy_result.c	(Arbeitskopie)
>@@ -77,13 +77,13 @@
> 			break;
> 		switch (c) {
> 			case 'h':
>-				ldap_parameters->host = strdup(optarg);
>+				ldap_parameters->host = optarg;
> 				break;
> 			case 'D':
>-				ldap_parameters->binddn = strdup(optarg);
>+				ldap_parameters->binddn = optarg;
> 				break;
> 			case 'w':
>-				ldap_parameters->bindpw = strdup(optarg);
>+				ldap_parameters->bindpw = optarg;
> 				break;
> 			case 'd':
> 				opt_debug = 1;
>@@ -111,7 +111,7 @@
> 		univention_debug_init("/dev/null", 0, 0);
> 	}
> 
>-	dn = argv[argc-1];
>+	dn = argv[argc - 1];
> 
> 	if (univention_ldap_open(ldap_parameters) != 0) {
> 		if (output == OUTPUT_VERBOSE) {
>@@ -155,7 +155,7 @@
> 					printf("\n");
> 				} else if (output == OUTPUT_SHELL) {
> 					for (i = 0; attribute->values->values[i] != NULL; i++) {
>-						for (j = 0; j<strlen(attribute->name); j++) {
>+						for (j = 0; j < strlen(attribute->name); j++) {
> 							if (attribute->name[j] == ';' || attribute->name[j] == '-') {
> 								printf("_");
> 							} else {
>@@ -181,7 +181,7 @@
> 					if (attribute != policy->attributes)
> 						printf(" ");
> 					for (i = 0; attribute->values->values[i] != NULL; i++) {
>-						if (i>0)
>+						if (i > 0)
> 							printf(" ");
> 						printf("%s=\"%s\"", attribute->name, attribute->values->values[i]);
> 					}
>Index: debian/changelog
>===================================================================
>--- debian/changelog	(Revision 24237)
>+++ debian/changelog	(Arbeitskopie)
>@@ -7,8 +7,12 @@
>   * Remove broken libunivention-policy-krb5-dev.
>   * Add manual page.
>   * Add linkting to liblber.
>+  * Fix memory leaks and NULL pointer exceptions.
>+  * Simplify allocating and cleaning memory.
>+  * Handle line terminattion in ldap.secret.
>+  * Make univention_krb5_init() thread-save.
> 
>- -- Philipp Hahn <hahn@univention.de>  Tue, 17 May 2011 18:34:11 +0200
>+ -- Philipp Hahn <hahn@univention.de>  Tue, 17 May 2011 19:19:37 +0200
> 
> univention-policy (4.0.2-1) unstable; urgency=low
> 
>Index: include/univention/policy.h
>===================================================================
>--- include/univention/policy.h	(Revision 24236)
>+++ include/univention/policy.h	(Arbeitskopie)
>@@ -60,8 +60,8 @@
> } univention_policy_handle_t;
> 
> 
>-univention_policy_handle_t* univention_policy_open(LDAP* ld, char* base, char* dn);
>-univention_policy_result_t* univention_policy_get(univention_policy_handle_t* handle, char* policy_name, char* attribute_name);
>+univention_policy_handle_t* univention_policy_open(LDAP *ld, const char *base, const char *dn);
>+univention_policy_result_t* univention_policy_get(univention_policy_handle_t *handle, const char *policy_name, const char *attribute_name);
> void univention_policy_close(univention_policy_handle_t* handle);
> 
> #endif
>Index: lib/policy.c
>===================================================================
>--- lib/policy.c	(Revision 24237)
>+++ lib/policy.c	(Arbeitskopie)
>@@ -53,7 +53,7 @@
> /*
>  * returns object from list if it already exists, create new object otherwise
>  */
>-struct univention_policy_list_s* univention_policy_list_get(struct univention_policy_list_s** list, char* name)
>+struct univention_policy_list_s* univention_policy_list_get(struct univention_policy_list_s** list, const char *name)
> {
> 	struct univention_policy_list_s *new;
> 	struct univention_policy_list_s *cur;
>@@ -82,7 +82,7 @@
> /*
>  * returns object from list if it already exists, create new object otherwise
>  */
>-struct univention_policy_attribute_list_s* univention_policy_attribute_list_get(struct univention_policy_attribute_list_s** list, char* name)
>+struct univention_policy_attribute_list_s* univention_policy_attribute_list_get(struct univention_policy_attribute_list_s **list, const char *name)
> {
> 	struct univention_policy_attribute_list_s *new;
> 	struct univention_policy_attribute_list_s *cur;
>@@ -99,10 +99,7 @@
> 	new->name = strdup(name);
> 	new->values = NULL;
> 
>-	if (*list == NULL)
>-		new->next = NULL;
>-	else
>-		new->next = *list;
>+	new->next = *list;
> 	*list = new;
> 
> 	return new;
>@@ -171,7 +168,7 @@
> 	}
> }
> 
>-void univention_policy_merge(LDAP* ld, char *dn, univention_policy_handle_t* handle, char** object_classes)
>+void univention_policy_merge(LDAP *ld, const char *dn, univention_policy_handle_t *handle, char **object_classes)
> {
> 	int		rc;
> 	LDAPMessage	*res;
>@@ -327,7 +324,7 @@
> /*
>  * reads policies for dn from conn
>  */
>-univention_policy_handle_t* univention_policy_open(LDAP* ld, char* base, char* dn)
>+univention_policy_handle_t* univention_policy_open(LDAP* ld, const char *base, const char *dn)
> {
> 	const char* pdn;
> 	int rc;
>@@ -356,7 +353,7 @@
> 	timeout.tv_usec = 0;
> 
> 	for (pdn = dn; pdn != NULL; pdn = parent_dn(pdn)) {
>-		char* filter;
>+		const char *filter;
> 
> 		univention_debug(UV_DEBUG_POLICY, UV_DEBUG_INFO, "processing dn %s", pdn);
> 
>@@ -369,8 +366,7 @@
> 			if ( rc == LDAP_NO_SUCH_OBJECT ) {
> 				univention_debug(UV_DEBUG_LDAP, UV_DEBUG_WARN, "Not found");
> 			} else if ( rc != LDAP_SUCCESS ) {
>-				univention_debug(UV_DEBUG_LDAP, UV_DEBUG_ERROR,
>-						"%s: %s", pdn, ldap_err2string(rc));
>+				univention_debug(UV_DEBUG_LDAP, UV_DEBUG_ERROR, "%s: %s", pdn, ldap_err2string(rc));
> 				univention_policy_close(handle);
> 				return NULL;
> 			}
>@@ -449,7 +445,7 @@
> /*
>  * returns values for policy/attribute
>  */
>-univention_policy_result_t* univention_policy_get(univention_policy_handle_t* handle, char* policy_name, char* attribute_name)
>+univention_policy_result_t* univention_policy_get(univention_policy_handle_t* handle, const char *policy_name, const char *attribute_name)
> {
> 	struct univention_policy_list_s* policy;
> 	struct univention_policy_attribute_list_s* attribute;
>Index: lib/ldap.c
>===================================================================
>--- lib/ldap.c	(Revision 24237)
>+++ lib/ldap.c	(Arbeitskopie)
>@@ -42,25 +42,19 @@
> #include <univention/ldap.h>
> #include <univention/debug.h>
> 
>+#define FREE(p) \
>+	do { \
>+		if (p != NULL) { \
>+			free(p); \
>+			p = NULL; \
>+		} \
>+	} while (0)
>+
> univention_ldap_parameters_t* univention_ldap_new(void)
> {
> 	univention_ldap_parameters_t* lp;
>-	if ((lp = malloc(sizeof(univention_ldap_parameters_t))) == NULL)
>+	if ((lp = calloc(1, sizeof(univention_ldap_parameters_t))) == NULL)
> 		return NULL;
>-	lp->ld = NULL;
>-	lp->version = 0;
>-	lp->host = NULL;
>-	lp->port = 0;
>-	lp->uri = NULL;
>-	lp->start_tls = 0;
>-	lp->base = NULL;
>-	lp->binddn = NULL;
>-	lp->bindpw = NULL;
>-	lp->authmethod = 0;
>-	lp->sasl_mech = NULL;
>-	lp->sasl_realm = NULL;
>-	lp->sasl_authcid = NULL;
>-	lp->sasl_authzid = NULL;
> 	return lp;
> }
> 
>@@ -113,50 +107,59 @@
> 	return LDAP_SUCCESS;
> }
> 
>+#define _UNIVENTION_LDAP_SECRET_LEN_MAX 27
> int univention_ldap_set_admin_connection( univention_ldap_parameters_t *lp )
> {
> 	FILE *secret;
>-	char *base    = NULL;
>+	char *base = NULL;
>+	size_t len;
> 
> 	base = univention_config_get_string("ldap/base");
>-	if ( !base ) {
>-		return 1;
>-	}
>-	lp->binddn = malloc( ( strlen(base) + strlen("cn=admin,") + 1) * sizeof (char) );
>-	if ( !lp->binddn ) {
>+	if (!base)
>+		goto err;
>+	len = strlen(base) + strlen("cn=admin,") + 1;
>+	lp->binddn = malloc(sizeof(char) * len);
>+	if (!lp->binddn) {
> 		free(base);
>-		return 1;
>+		goto err;
> 	}
>-	sprintf(lp->binddn, "cn=admin,%s", base );
>+	snprintf(lp->binddn, len, "cn=admin,%s", base);
> 
> 	free(base);
> 
> 	secret = fopen("/etc/ldap.secret", "r" );
>+	if (!secret)
>+		goto err1;
> 
>-	if ( !secret ) {
>-		return 1;
>+	lp->bindpw = calloc(_UNIVENTION_LDAP_SECRET_LEN_MAX, sizeof(char));
>+	if (!lp->bindpw) {
>+		fclose(secret);
>+		goto err1;
> 	}
> 
>-	lp->bindpw = malloc(25*sizeof(char));
>+	len = fread(lp->bindpw, _UNIVENTION_LDAP_SECRET_LEN_MAX, sizeof(char), secret);
>+	if (ferror(secret))
>+		len = -1;
>+	fclose(secret);
> 
>-	if ( !lp->bindpw ) {
>-		return 1;
>+	for (; len >= 0; len--) {
>+		switch (lp->bindpw[len]) {
>+			case '\r':
>+			case '\n':
>+				lp->bindpw[len] = '\0';
>+			case '\0':
>+				continue;
>+			default:
>+				return 0;
>+		}
> 	}
> 
>-	memset(lp->bindpw, 0, 25);
>-
>-	fread(lp->bindpw, 24, 1, secret);
>-
>-	if ( lp->bindpw[strlen(lp->bindpw)-1] == '\r' ) {
>-		lp->bindpw[strlen(lp->bindpw)-1] = '\0';
>-	}
>-	if ( lp->bindpw[strlen(lp->bindpw)-1] == '\n' ) {
>-		lp->bindpw[strlen(lp->bindpw)-1] = '\0';
>-	}
>-
>-	fclose(secret);
>-
>-	return 0;
>+err2:
>+	FREE(lp->bindpw);
>+err1:
>+	FREE(lp->binddn);
>+err:
>+	return 1;
> }
> 
> int univention_ldap_open(univention_ldap_parameters_t *lp)
>@@ -281,44 +284,14 @@
> 		ldap_unbind_ext(lp->ld, NULL, NULL);
> 		lp->ld = NULL;
> 	}
>-	if (lp->uri != NULL) {
>-		free(lp->uri);
>-		lp->uri = NULL;
>-	}
>-	if (lp->host != NULL) {
>-		free(lp->host);
>-		lp->host = NULL;
>-	}
>-	if (lp->base != NULL) {
>-		free(lp->base);
>-		lp->base = NULL;
>-	}
>-	if (lp->binddn != NULL) {
>-		free(lp->binddn);
>-		lp->binddn = NULL;
>-	}
>-	if (lp->bindpw != NULL) {
>-		free(lp->bindpw);
>-		lp->bindpw = NULL;
>-	}
>-	if (lp->sasl_mech != NULL) {
>-		free(lp->sasl_mech);
>-		lp->sasl_mech = NULL;
>-	}
>-	if (lp->sasl_realm != NULL) {
>-		free(lp->sasl_realm);
>-		lp->sasl_realm = NULL;
>-	}
>-	if (lp->sasl_authcid != NULL) {
>-		free(lp->sasl_authcid);
>-		lp->sasl_authcid = NULL;
>-	}
>-	if (lp->sasl_authzid != NULL) {
>-		free(lp->sasl_authzid);
>-		lp->sasl_authzid = NULL;
>-	}
>-	if (lp != NULL) {
>-		free(lp);
>-		lp = NULL;
>-	}
>+	FREE(lp->uri);
>+	FREE(lp->host);
>+	FREE(lp->base);
>+	FREE(lp->binddn);
>+	FREE(lp->bindpw);
>+	FREE(lp->sasl_mech);
>+	FREE(lp->sasl_realm);
>+	FREE(lp->sasl_authcid);
>+	FREE(lp->sasl_authzid);
>+	FREE(lp);
> }
>Index: lib/krb5.c
>===================================================================
>--- lib/krb5.c	(Revision 24237)
>+++ lib/krb5.c	(Arbeitskopie)
>@@ -44,11 +44,8 @@
> univention_krb5_parameters_t* univention_krb5_new(void)
> {
> 	univention_krb5_parameters_t* kp;
>-	if ((kp = malloc(sizeof(univention_krb5_parameters_t))) == NULL)
>+	if ((kp = calloc(1, sizeof(univention_krb5_parameters_t))) == NULL)
> 		return NULL;
>-	kp->username = NULL;
>-	kp->realm = NULL;
>-	kp->password = NULL;
> 	return kp;
> }
> 
>@@ -73,68 +70,69 @@
> 
> int univention_krb5_init(univention_krb5_parameters_t *kp)
> {
>-	krb5_error_code rv;
>+	krb5_error_code rv = -1;
> 	char *principal_name;
> 
> 	if (kp->username == NULL) {
>-		struct passwd *pwd;
>-		pwd = getpwuid(getuid());
>-		if (pwd == NULL) {
>-			return 1;
>+		struct passwd pwd, *result;
>+		char *buf;
>+		size_t bufsize;
>+		int s;
>+
>+		bufsize = sysconf(_SC_GETPW_R_SIZE_MAX);
>+		if (bufsize == -1)
>+			bufsize = 16384;
>+		buf = malloc(bufsize);
>+		if (buf == NULL)
>+			goto err;
> 		}
>-		kp->username = strdup(pwd->pw_name);
>+		s = getpwnam_r(argv[1], &pwd, buf, bufsize, &result);
>+		if (result != NULL)
>+			kp->username = strdup(pwd.pw_name);
>+		free(buf);
> 	}
>-	if (kp->realm == NULL) {
>+
>+	if (kp->realm == NULL)
> 		kp->realm = univention_config_get_string("kerberos/realm");
>-		if (kp->realm == NULL) {
>-			return 1;
>-		}
>-	}
>+
>+	if (kp->username == NULL || kp->realm == NULL)
>+		goto err;
> 	asprintf(&principal_name, "%s@%s", kp->username, kp->realm);
>+	if (principal_name == NULL)
>+		goto err;
> 
> 	univention_debug(UV_DEBUG_KERBEROS, UV_DEBUG_INFO, "receiving Kerberos ticket for %s", principal_name);
> 
>-	if ((rv = krb5_init_context(&kp->context))) {
>-		free(principal_name);
>-		return rv;
>-	}
>-	if ((rv = krb5_cc_default(kp->context, &kp->ccache))) {
>-		free(principal_name);
>-		krb5_free_context(kp->context);
>-		return rv;
>-	}
>-	if ((rv = krb5_parse_name(kp->context, principal_name, &kp->principal))) {
>-		free(principal_name);
>-		krb5_free_context(kp->context);
>-		return rv;
>-	}
>+	if ((rv = krb5_init_context(&kp->context)))
>+		goto err1;
>+	if ((rv = krb5_cc_default(kp->context, &kp->ccache)))
>+		goto err2;
>+	if ((rv = krb5_parse_name(kp->context, principal_name, &kp->principal)))
>+		goto err2;
> 	if ((rv = krb5_get_init_creds_password(kp->context, &kp->creds, kp->principal,
>-					NULL, kerb_prompter, kp->password, 0, NULL, NULL))) {
>-		free(principal_name);
>-		krb5_free_principal(kp->context, kp->principal);
>-		krb5_free_context(kp->context);
>-		return rv;
>-	}
>-	if ((rv = krb5_cc_initialize(kp->context, kp->ccache, kp->principal))) {
>-		free(principal_name);
>-		krb5_free_cred_contents(kp->context, &kp->creds);
>-		krb5_free_principal(kp->context, kp->principal);
>-		krb5_free_context(kp->context);
>-		return rv;
>-	}
>-	if ((rv = krb5_cc_store_cred(kp->context, kp->ccache, &kp->creds))) {
>-		free(principal_name);
>-		krb5_cc_close(kp->context, kp->ccache);
>-		krb5_free_cred_contents(kp->context, &kp->creds);
>-		krb5_free_principal(kp->context, kp->principal);
>-		krb5_free_context(kp->context);
>-		return rv;
>-	}
>+					NULL, kerb_prompter, kp->password, 0, NULL, NULL)))
>+		goto err3;
>+	if ((rv = krb5_cc_initialize(kp->context, kp->ccache, kp->principal)))
>+		goto err4;
>+	if ((rv = krb5_cc_store_cred(kp->context, kp->ccache, &kp->creds)))
>+		goto err5;
> 
>-	free(principal_name);
>+	rv = 0;
>+
>+err5:
> 	krb5_cc_close(kp->context, kp->ccache);
>+	kp->ccache = NULL;
>+err4:
> 	krb5_free_cred_contents(kp->context, &kp->creds);
>+	kp->creds = NULL;
>+err3:
> 	krb5_free_principal(kp->context, kp->principal);
>+	kp->principal = NULL;
>+err2:
> 	krb5_free_context(kp->context);
>-	return 0;
>+	kp->context = NULL;
>+err1:
>+	free(principal_name);
>+err:
>+	return rv;
> }

Actions: View | Diff

Attachments on bug 22553: 3281