[SEMANAGE] [SETSEBOOL] Clone record on set/add/modify

[SEMANAGE] [SETSEBOOL] Clone record on set/add/modify

[Ivan Gyurdiev]

[-- Attachment #1: Type: text/plain, Size: 576 bytes --]

Hi, Steven pointed out (off-list) that libsemanage vs libsepol behavior 
is inconsistent with respect to set/add/modify.
In libsemanage, the object being added becomes "managed" by the library, 
while this is not true in libsepol. We decided it was better to have the 
client control the object passed in, and be responsible for freeing it.

This patch makes the appropriate changes (which include reversal of some 
of the code from the other patches) to implement that.
It also includes a bugfix for the error path of 
database_policydb.c:list, pointed out by Russel Coker.


[-- Attachment #2: libsemanage17.setsebool3.caller_free_on_set.diff --]
[-- Type: text/x-patch, Size: 7308 bytes --]

diff -Naurp --exclude-from excludes old/libsemanage/src/database_activedb.c new/libsemanage/src/database_activedb.c
--- old/libsemanage/src/database_activedb.c	2005-12-23 04:50:26.000000000 -0500
+++ new/libsemanage/src/database_activedb.c	2006-01-04 08:34:16.000000000 -0500
@@ -54,7 +54,7 @@ static int dbase_activedb_cache(
 	for (i = 0; i < rcount; i++) { 
 		if (dbase_llist_cache_prepend(handle, &dbase->llist, records[i]) < 0)
 			goto err;
-		records[i] = NULL;
+		rtable->free(records[i]);
 	}
 
 	free(records);
diff -Naurp --exclude-from excludes old/libsemanage/src/database_file.c new/libsemanage/src/database_file.c
--- old/libsemanage/src/database_file.c	2005-12-23 01:01:49.000000000 -0500
+++ new/libsemanage/src/database_file.c	2006-01-04 08:16:06.000000000 -0500
@@ -107,6 +107,7 @@ static int dbase_file_cache(
 			process_record) < 0)
 			goto err;
 
+		rtable->free(process_record);
 		process_record = NULL;
 
 	} while (pstatus != STATUS_NODATA);
diff -Naurp --exclude-from excludes old/libsemanage/src/database.h new/libsemanage/src/database.h
--- old/libsemanage/src/database.h	2006-01-03 07:01:09.000000000 -0500
+++ new/libsemanage/src/database.h	2006-01-04 08:17:28.000000000 -0500
@@ -62,14 +62,13 @@ typedef struct dbase_table {
 	/* --------------- Database Functionality ----------- */
 
 	/* Note: In all the functions below, the key is property
-	 * of the caller, and will not be modified by the database. */
+	 * of the caller, and will not be modified by the database. 
+	 * In add/set/modify, the data is also property of the caller */
 
 	/* Add the specified record to
 	 * the database if it is not present,
 	 * or fail if it already exists
-	 * 
-	 * Note: the <data> record becomes property of the 
-	 * database, and may not be further modified */
+	 */
 	int (*add) (
 		struct semanage_handle* handle,
 		dbase_t* dbase,
@@ -79,9 +78,7 @@ typedef struct dbase_table {
 	/* Add the specified record to the  
 	 * database if it not present. 
 	 * If it's present, replace it
-	 * 
-	 * Note: The <data> record becomes property of the
-	 * database, and may not be further modified */
+	 */
 	int (*modify) (
 		struct semanage_handle* handle,
 		dbase_t* dbase,
@@ -90,9 +87,7 @@ typedef struct dbase_table {
 
 	/* Modify the specified record in the database
 	 * if it is present. Fail if it does not yet exist
-	 *
-	 * Note: The <data> record becomes property of the
-	 * database, and may not be further modified*/
+	 */
 	int (*set) (
 		struct semanage_handle* handle,
 		dbase_t* dbase,
diff -Naurp --exclude-from excludes old/libsemanage/src/database_llist.c new/libsemanage/src/database_llist.c
--- old/libsemanage/src/database_llist.c	2005-12-26 21:21:34.000000000 -0500
+++ new/libsemanage/src/database_llist.c	2006-01-04 08:40:25.000000000 -0500
@@ -24,7 +24,10 @@ hidden int dbase_llist_cache_prepend(
 		(cache_entry_t*) malloc(sizeof (cache_entry_t));
 	if (entry == NULL)
 		goto omem;
-	entry->data = data;
+	
+	if (dbase->rtable->clone(handle, data, &entry->data) < 0)
+		goto err;
+
 	entry->prev = NULL;
 	entry->next = dbase->cache;
 
@@ -38,8 +41,12 @@ hidden int dbase_llist_cache_prepend(
         return STATUS_SUCCESS;
 
         omem:
-	ERR(handle, "out of memory, could not cache record");
-        return STATUS_ERR;
+	ERR(handle, "out of memory");
+
+	err:
+	ERR(handle, "could not cache record");
+	free(entry);
+	return STATUS_ERR;
 }
 
 hidden void dbase_llist_drop_cache(
@@ -154,7 +161,8 @@ hidden int dbase_llist_set(
 	}
 	else {
 		dbase->rtable->free(entry->data);
-		entry->data = data;
+		if (dbase->rtable->clone(handle, data, &entry->data) < 0)
+			goto err;
 	}
 
 	dbase->modified = 1;
@@ -184,7 +192,8 @@ hidden int dbase_llist_modify(
 	}
 	else {
 		dbase->rtable->free(entry->data);
-		entry->data = data;
+		if (dbase->rtable->clone(handle, data, &entry->data) < 0)
+			goto err;
 	}
 
 	dbase->modified = 1;
diff -Naurp --exclude-from excludes old/libsemanage/src/database_policydb.c new/libsemanage/src/database_policydb.c
--- old/libsemanage/src/database_policydb.c	2005-12-23 01:01:49.000000000 -0500
+++ new/libsemanage/src/database_policydb.c	2006-01-04 08:41:06.000000000 -0500
@@ -375,7 +375,7 @@ static int dbase_policydb_list (
 	size_t* count) {
 
 	record_t** tmp_records = NULL;
-	size_t tmp_count, i;
+	size_t tmp_count;
 	struct list_handler_arg list_arg;
 	list_arg.pos = 0;
 	list_arg.rtable = dbase->rtable;
@@ -410,7 +410,7 @@ static int dbase_policydb_list (
 
 	err:
 	for (; list_arg.pos >= 0; list_arg.pos--)
-		dbase->rtable->free(tmp_records[i]);
+		dbase->rtable->free(tmp_records[list_arg.pos]);
 	free(tmp_records);
 	ERR(handle, "could not list records");
 	return STATUS_ERR;
diff -Naurp --exclude-from excludes old/libsemanage/src/policy_components.c new/libsemanage/src/policy_components.c
--- old/libsemanage/src/policy_components.c	2006-01-03 06:40:03.000000000 -0500
+++ new/libsemanage/src/policy_components.c	2006-01-04 08:22:06.000000000 -0500
@@ -72,7 +72,6 @@ static int load_handler(
 	void* varg) {
 
 	record_key_t* rkey = NULL;
-	record_t* rcopy = NULL;
 	load_handler_arg_t* arg = 
 		(load_handler_arg_t*) varg;
 
@@ -84,22 +83,17 @@ static int load_handler(
 	if (rtable->key_extract(handle, record, &rkey) < 0)
 		goto err;
 
-	if (rtable->clone(handle, record, &rcopy) < 0)
-		goto err;
- 
 	switch (arg->mode) {
 	
 		case MODE_SET:
-			if (dtable->set(handle, dbase, rkey, rcopy) < 0) 
+			if (dtable->set(handle, dbase, rkey, record) < 0) 
 				goto err;
-			rcopy = NULL;
 			break;
 		
 		default:
 		case MODE_MODIFY:
-			if (dtable->modify(handle, dbase, rkey, rcopy) < 0) 
+			if (dtable->modify(handle, dbase, rkey, record) < 0) 
 				goto err;
-			rcopy = NULL;
 			break;
 
 	}
@@ -110,7 +104,6 @@ static int load_handler(
 	err:
 	/* FIXME: handle error */
 	rtable->key_free(rkey);
-	rtable->free(rcopy);
 	return -1;
 }
 
diff -Naurp --exclude-from excludes old/policycoreutils/setsebool/setsebool.c new/policycoreutils/setsebool/setsebool.c
--- old/policycoreutils/setsebool/setsebool.c	2005-12-23 18:44:43.000000000 -0500
+++ new/policycoreutils/setsebool/setsebool.c	2006-01-04 08:19:53.000000000 -0500
@@ -102,7 +102,6 @@ static int semanage_set_boolean_list(
 	size_t j;
 	semanage_handle_t* handle = NULL;
 	semanage_bool_t* boolean = NULL;
-	semanage_bool_t* boolean2 = NULL;
 	semanage_bool_key_t* bool_key = NULL;
 	int managed;
 
@@ -142,19 +141,14 @@ static int semanage_set_boolean_list(
 		if (semanage_bool_key_extract(handle, boolean, &bool_key) < 0)
 			goto err;
 
-		if (permanent) {
-			if (semanage_bool_clone(handle, boolean, &boolean2) < 0)
-				goto err;
-
-			if (semanage_bool_modify_local(handle, bool_key, boolean2) < 0)
-				goto err;
-			boolean2 = NULL;
-		} 
+		if (permanent && semanage_bool_modify_local(handle, bool_key, boolean) < 0)
+			goto err;
 
 		if (semanage_bool_set_active(handle, bool_key, boolean) < 0)
 			goto err;
 
 		semanage_bool_key_free(bool_key);
+		semanage_bool_free(boolean);
 		bool_key = NULL;
 		boolean = NULL;
 	}	
@@ -172,7 +166,6 @@ static int semanage_set_boolean_list(
 	err:
 	semanage_bool_key_free(bool_key);
 	semanage_bool_free(boolean);
-	semanage_bool_free(boolean2);
 	semanage_handle_destroy(handle);
 	fprintf(stderr, "Could not change policy booleans\n");
 	return -1;

Re: [SEMANAGE] [SETSEBOOL] Clone record on set/add/modify

[Ivan Gyurdiev]

Ivan Gyurdiev wrote:
> Hi, Steven pointed out (off-list) that libsemanage vs libsepol 
> behavior is inconsistent with respect to set/add/modify.
> In libsemanage, the object being added becomes "managed" by the 
> library, while this is not true in libsepol. We decided it was better 
> to have the client control the object passed in, and be responsible 
> for freeing it.
>
> This patch makes the appropriate changes (which include reversal of 
> some of the code from the other patches) to implement that.
> It also includes a bugfix for the error path of 
> database_policydb.c:list, pointed out by Russel Coker.
>
By the way, how does python handle this - does it manage memory 
management automatically, or do we still need to call the free 
functions.... because.. I don't see any free functions being called in 
the semanage tool - not even for the key.


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [SEMANAGE] [SETSEBOOL] Clone record on set/add/modify

[Stephen Smalley]

On Wed, 2006-01-04 at 09:08 -0500, Ivan Gyurdiev wrote:
> Hi, Steven pointed out (off-list) that libsemanage vs libsepol behavior 
> is inconsistent with respect to set/add/modify.
> In libsemanage, the object being added becomes "managed" by the library, 
> while this is not true in libsepol. We decided it was better to have the 
> client control the object passed in, and be responsible for freeing it.
> 
> This patch makes the appropriate changes (which include reversal of some 
> of the code from the other patches) to implement that.
> It also includes a bugfix for the error path of 
> database_policydb.c:list, pointed out by Russel Coker.

Thanks, your entire patch series has been merged as of libsepol 1.11.2,
libsemanage 1.5.4, and policycoreutils 1.29.3.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [SEMANAGE] [SETSEBOOL] Clone record on set/add/modify

[Ivan Gyurdiev]

> Thanks, your entire patch series has been merged as of libsepol 1.11.2,
> libsemanage 1.5.4, and policycoreutils 1.29.3.
>   
Question - I did some reordering of the headers in semanageswig.i (at 
the beginning).
I see you haven't merged that part, but you're missing a header that I 
added - booleans_active.h (which is why I did the reordering actually - 
not to miss any headers). Of course, semanage.h includes all of those 
headers, so I'm not sure whether or not any of them are needed in the 
first place, given that semanage.h is included.


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [SEMANAGE] [SETSEBOOL] Clone record on set/add/modify

[Stephen Smalley]

On Wed, 2006-01-04 at 11:12 -0500, Ivan Gyurdiev wrote:
> > Thanks, your entire patch series has been merged as of libsepol 1.11.2,
> > libsemanage 1.5.4, and policycoreutils 1.29.3.
> >   
> Question - I did some reordering of the headers in semanageswig.i (at 
> the beginning).
> I see you haven't merged that part, but you're missing a header that I 
> added - booleans_active.h (which is why I did the reordering actually - 
> not to miss any headers). Of course, semanage.h includes all of those 
> headers, so I'm not sure whether or not any of them are needed in the 
> first place, given that semanage.h is included.

Ah, I had some rejects on that file, and tried resolving them manually,
but I missed that.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [SEMANAGE] [SETSEBOOL] Clone record on set/add/modify

[Ivan Gyurdiev]

[-- Attachment #1: Type: text/plain, Size: 835 bytes --]

Stephen Smalley wrote:
> On Wed, 2006-01-04 at 11:12 -0500, Ivan Gyurdiev wrote:
>   
>>> Thanks, your entire patch series has been merged as of libsepol 1.11.2,
>>> libsemanage 1.5.4, and policycoreutils 1.29.3.
>>>   
>>>       
>> Question - I did some reordering of the headers in semanageswig.i (at 
>> the beginning).
>> I see you haven't merged that part, but you're missing a header that I 
>> added - booleans_active.h (which is why I did the reordering actually - 
>> not to miss any headers). Of course, semanage.h includes all of those 
>> headers, so I'm not sure whether or not any of them are needed in the 
>> first place, given that semanage.h is included.
>>     
>
> Ah, I had some rejects on that file, and tried resolving them manually,
> but I missed that.
>   
Well, here's my current diff against that file..



[-- Attachment #2: libsemanage.swig_headers.diff --]
[-- Type: text/x-patch, Size: 1954 bytes --]

diff -Naurp --exclude man --exclude-from excludes old/libsemanage/src/semanageswig.i new/libsemanage/src/semanageswig.i
--- old/libsemanage/src/semanageswig.i	2006-01-04 10:54:48.000000000 -0500
+++ new/libsemanage/src/semanageswig.i	2006-01-03 05:38:30.000000000 -0500
@@ -20,31 +20,29 @@
 
 %module semanage
 %{
-	#include "semanage/boolean_record.h"
 	#include "semanage/debug.h"
-	#include "semanage/interfaces_policy.h" 
-	#include "semanage/ports_policy.h"      
-	#include "semanage/booleans_local.h"     
-	#include "semanage/handle.h"             
-	#include "semanage/modules.h"            
-	#include "semanage/semanage.h"           
+	#include "semanage/handle.h"
+	#include "semanage/modules.h"
+	#include "semanage/context_record.h"
+	#include "semanage/boolean_record.h"
+	#include "semanage/booleans_policy.h"
+	#include "semanage/booleans_local.h"
+	#include "semanage/booleans_active.h"
+	#include "semanage/iface_record.h"
+	#include "semanage/interfaces_local.h"
+	#include "semanage/interfaces_policy.h"
 	#include "semanage/user_record.h"
-	#include "semanage/booleans_policy.h"    
-	#include "semanage/iface_record.h"       
-	#include "semanage/port_record.h"        
-	#include "semanage/seuser_record.h"
- 	#include "semanage/fcontext_record.h"
- 	#include "semanage/fcontexts_local.h"
-	#include "semanage/fcontexts_policy.h"
 	#include "semanage/users_local.h"
-	#include "semanage/context_record.h"     
-	#include "semanage/interfaces_local.h"   
-	#include "semanage/ports_local.h"        
-	#include "semanage/seusers.h"            
 	#include "semanage/users_policy.h"
 	#include "semanage/port_record.h"
 	#include "semanage/ports_local.h"
 	#include "semanage/ports_policy.h"
+	#include "semanage/seuser_record.h"
+	#include "semanage/fcontext_record.h"
+	#include "semanage/fcontexts_local.h"
+	#include "semanage/fcontexts_policy.h"
+	#include "semanage/seusers.h"	
+	#include "semanage/semanage.h"
 %}
 
 %inline {

Re: [SEMANAGE] [SETSEBOOL] Clone record on set/add/modify

[Ivan Gyurdiev]

[-- Attachment #1: Type: text/plain, Size: 432 bytes --]


> Well, the add function is broken, as you noted in your other mail, but 
> why it crashes I have no idea.
Here's a patch to fix the add function. Typically I test modify and 
assume add/set work - they're mostly the same.
I guess assumptions are a bad thing. Before Joshua says "I told you so" 
- I will update pywrap-test.py by tomorrow.

Now I'm curious to investigate the segfault - that should most 
definitely *not* happen.


[-- Attachment #2: libsemanage.toggle_modify.diff --]
[-- Type: text/x-patch, Size: 456 bytes --]

diff -Naurp --exclude man --exclude-from excludes old/libsemanage/src/database_llist.c new/libsemanage/src/database_llist.c
--- old/libsemanage/src/database_llist.c	2006-01-04 12:18:17.000000000 -0500
+++ new/libsemanage/src/database_llist.c	2006-01-04 14:16:49.000000000 -0500
@@ -136,6 +136,7 @@ hidden int dbase_llist_add(
 	if (dbase_llist_cache_prepend(handle, dbase, data) < 0)
 		goto err;
 
+	dbase->modified = 1;
 	return STATUS_SUCCESS;
 
 	err:

Re: [SEMANAGE] [SETSEBOOL] Clone record on set/add/modify

[Joshua Brindle]

Stephen Smalley wrote:
> On Wed, 2006-01-04 at 09:08 -0500, Ivan Gyurdiev wrote:
> 
>>Hi, Steven pointed out (off-list) that libsemanage vs libsepol behavior 
>>is inconsistent with respect to set/add/modify.
>>In libsemanage, the object being added becomes "managed" by the library, 
>>while this is not true in libsepol. We decided it was better to have the 
>>client control the object passed in, and be responsible for freeing it.
>>
>>This patch makes the appropriate changes (which include reversal of some 
>>of the code from the other patches) to implement that.
>>It also includes a bugfix for the error path of 
>>database_policydb.c:list, pointed out by Russel Coker.
> 
> 
> Thanks, your entire patch series has been merged as of libsepol 1.11.2,
> libsemanage 1.5.4, and policycoreutils 1.29.3.
> 

Ivan, were you going to add tests for the wrappers to pywrap-test?

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [SEMANAGE] [SETSEBOOL] Clone record on set/add/modify

[Ivan Gyurdiev]

> Ivan, were you going to add tests for the wrappers to pywrap-test?
...eventually. Right now I'm writing manpages,  and looking of the 
semanage(tool) diff.
I promise it works :)
(and if it doesn't, I'll get it fixed).

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [SEMANAGE] [SETSEBOOL] Clone record on set/add/modify

[Joshua Brindle]

Ivan Gyurdiev wrote:
> 
>> Ivan, were you going to add tests for the wrappers to pywrap-test?
> 
> ...eventually. Right now I'm writing manpages,  and looking of the 
> semanage(tool) diff.
> I promise it works :)
> (and if it doesn't, I'll get it fixed).
> 

not the point.

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.