Re: [389-devel] Please Review: (514824) Multi-macro ACIs can cause double free if macro attribute does not exist

[Nathan Kinder <nkinder@xxxxxxxxxx>]

On 07/30/2009 03:37 PM, Nathan Kinder wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=514824

--
389-devel mailing list
389-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/fedora-directory-devel
  

Here's a new patch that addresses some issues Noriko brought up.

The list "a" was being set to NULL when we found an attribute match for a
macro, but this is no longer necessary now that we reset "a" to NULL when the
memory is handed off to the working_list (which covers both the cases of
finding/not finding the attribute).

We were also accessing element 0 of list "a" right after handing the memory off
to the working_list, but we weren't checking if "a" was NULL first.  I don't
believe that "a" could be NULL at this point, but it's safest to check first in
case there is some corner case we're not considering. 

-NGK

>From b1c7eacf47f7e068260fac7d3c372d3fcb82f82a Mon Sep 17 00:00:00 2001
From: Nathan Kinder <nkinder@xxxxxxxxxxxxxxxxxxx>
Date: Thu, 30 Jul 2009 16:52:26 -0700
Subject: [PATCH] Bug 514824: Fix double free in macro ACI code.

If you have an ACI with multiple macros in it and the second attribtue does not
exist in the entry you are bound as, the in-memory list used for macro
substitution is free'd twice.

The code swaps hands the charray it plans to return after substitution over to
a working list, but it doesn't set the return list to NULL.  When the second
macro attribute is not found, the working list is free'd, yet the address is
returned to the caller, who then tries to free the list a second time.  The fix
is to set the list to be returned to NULL when the memory is handed over to the
working list.
---
 ldap/servers/plugins/acl/acllas.c |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/ldap/servers/plugins/acl/acllas.c b/ldap/servers/plugins/acl/acllas.c
index e16fbd2..d220596 100644
--- a/ldap/servers/plugins/acl/acllas.c
+++ b/ldap/servers/plugins/acl/acllas.c
@@ -4025,7 +4025,6 @@ acllas_replace_attr_macro( char *rule, lasInfo *lasinfo) {
                	int i, j;
 				char *patched_rule;
 
-				a = NULL;
 	            i= slapi_attr_first_value ( attr, &sval );
     	        while(i != -1) {
         	    	attrValue = slapi_value_get_berval(sval);
@@ -4045,12 +4044,23 @@ acllas_replace_attr_macro( char *rule, lasInfo *lasinfo) {
 
 				/*
 				 * Here, a is working_list, where each member has had
-				 * macro_str replaced with attrVal.
-				*/
+				 * macro_str replaced with attrVal.  We hand a over,
+				 * so we must set it to NULL since the working list
+				 * may be free'd later. */
 
 				charray_free(working_list);
-				working_list = a;
-				working_rule = a[0];
+				if (a == NULL) {
+					/* This shouldn't happen, but we play
+					 * if safe to avoid any problems. */
+					slapi_ch_free((void **)&macro_str);
+					slapi_ch_free((void **)&macro_attr_name);
+					charray_add(&a, slapi_ch_strdup(""));
+					return(a);
+				} else {
+					working_list = a;
+					working_rule = a[0];
+					a = NULL;
+				}
 			}
 			slapi_ch_free((void **)&macro_str);
 			slapi_ch_free((void **)&macro_attr_name);
-- 
1.6.2.5

--
389-devel mailing list
389-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/fedora-directory-devel