[PATCH] mcstransd: fix reload issue

[PATCH] mcstransd: fix reload issue

[wenzong.fan]

From: Wenzong Fan <wenzong.fan@windriver.com>

Generally mcstransd works well on MLS enabled system, but if "Reload
Translations" triggered, it will fail to translate any MLS level.

* Why it fails:

Check process_trans() in mcstrans.c:

 723 process_trans(char *buffer) {
 724         static domain_t *domain;
 [snip] ...
 784         if (!domain) {
 785                 domain = create_domain("Default");

While reloading translations, the struct of domain will be destroyed
but the static pointer will be kept, it becomes wild and prevents the
create_domain() from running; Then invalid domain will be used for
initializing hashtable that stores translations data.

* Fix to it:

Define local *domain to get the struct of domain always be initialized;
Use static hashtable to store all translations data.

Signed-off-by: Wenzong Fan <wenzong.fan@windriver.com>
---
 policycoreutils/mcstrans/src/mcstrans.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/policycoreutils/mcstrans/src/mcstrans.c b/policycoreutils/mcstrans/src/mcstrans.c
index 4d31857..e8eda32 100644
--- a/policycoreutils/mcstrans/src/mcstrans.c
+++ b/policycoreutils/mcstrans/src/mcstrans.c
@@ -100,11 +100,14 @@ typedef struct base_classification {
 	struct base_classification *next;
 } base_classification_t;
 
+static context_map_node_t *table_raw_to_trans[N_BUCKETS];
+static context_map_node_t *table_trans_to_raw[N_BUCKETS];
+
 typedef struct domain {
 	char *name;
 
-	context_map_node_t *raw_to_trans[N_BUCKETS];
-	context_map_node_t *trans_to_raw[N_BUCKETS];
+	context_map_node_t **raw_to_trans;
+	context_map_node_t **trans_to_raw;
 
 	base_classification_t *base_classifications;
 	word_group_t *groups;
@@ -643,9 +646,11 @@ add_cache(domain_t *domain, char *raw, char *trans) {
 	}
 
 	log_debug(" add_cache (%s,%s)\n", raw, trans);
+	domain->raw_to_trans = table_raw_to_trans;
 	if (add_to_hashtable(domain->raw_to_trans, map->raw, map) < 0)
 		goto err;
 
+	domain->trans_to_raw = table_trans_to_raw;
 	if (add_to_hashtable(domain->trans_to_raw, map->trans, map) < 0)
 		goto err;
 
@@ -721,7 +726,7 @@ static int read_translations(const char *filename);
  */
 static int
 process_trans(char *buffer) {
-	static domain_t *domain;
+	domain_t *domain;
 	static word_group_t *group;
 	static int base_classification;
 	static int lineno = 0;
-- 
1.9.1

Re: [PATCH] mcstransd: fix reload issue

[Stephen Smalley]

On 05/12/2015 10:11 PM, wenzong.fan@windriver.com wrote:
> From: Wenzong Fan <wenzong.fan@windriver.com>
> 
> Generally mcstransd works well on MLS enabled system, but if "Reload
> Translations" triggered, it will fail to translate any MLS level.
> 
> * Why it fails:
> 
> Check process_trans() in mcstrans.c:
> 
>  723 process_trans(char *buffer) {
>  724         static domain_t *domain;
>  [snip] ...
>  784         if (!domain) {
>  785                 domain = create_domain("Default");
> 
> While reloading translations, the struct of domain will be destroyed
> but the static pointer will be kept, it becomes wild and prevents the
> create_domain() from running; Then invalid domain will be used for
> initializing hashtable that stores translations data.
> 
> * Fix to it:
> 
> Define local *domain to get the struct of domain always be initialized;
> Use static hashtable to store all translations data.
> 
> Signed-off-by: Wenzong Fan <wenzong.fan@windriver.com>
> ---
>  policycoreutils/mcstrans/src/mcstrans.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/policycoreutils/mcstrans/src/mcstrans.c b/policycoreutils/mcstrans/src/mcstrans.c
> index 4d31857..e8eda32 100644
> --- a/policycoreutils/mcstrans/src/mcstrans.c
> +++ b/policycoreutils/mcstrans/src/mcstrans.c
> @@ -100,11 +100,14 @@ typedef struct base_classification {
>  	struct base_classification *next;
>  } base_classification_t;
>  
> +static context_map_node_t *table_raw_to_trans[N_BUCKETS];
> +static context_map_node_t *table_trans_to_raw[N_BUCKETS];
> +
>  typedef struct domain {
>  	char *name;
>  
> -	context_map_node_t *raw_to_trans[N_BUCKETS];
> -	context_map_node_t *trans_to_raw[N_BUCKETS];
> +	context_map_node_t **raw_to_trans;
> +	context_map_node_t **trans_to_raw;
>  
>  	base_classification_t *base_classifications;
>  	word_group_t *groups;
> @@ -643,9 +646,11 @@ add_cache(domain_t *domain, char *raw, char *trans) {
>  	}
>  
>  	log_debug(" add_cache (%s,%s)\n", raw, trans);
> +	domain->raw_to_trans = table_raw_to_trans;
>  	if (add_to_hashtable(domain->raw_to_trans, map->raw, map) < 0)
>  		goto err;
>  
> +	domain->trans_to_raw = table_trans_to_raw;
>  	if (add_to_hashtable(domain->trans_to_raw, map->trans, map) < 0)
>  		goto err;
>  
> @@ -721,7 +726,7 @@ static int read_translations(const char *filename);
>   */
>  static int
>  process_trans(char *buffer) {
> -	static domain_t *domain;
> +	domain_t *domain;

Not really familiar with this code, but if you make this change, then
where is domain initialized prior to first use?

>  	static word_group_t *group;
>  	static int base_classification;
>  	static int lineno = 0;

And why is it ok for these to remain static?

Re: [PATCH] mcstransd: fix reload issue

[wenzong fan]

On 05/15/2015 12:43 AM, Stephen Smalley wrote:
> On 05/12/2015 10:11 PM, wenzong.fan@windriver.com wrote:
>> From: Wenzong Fan <wenzong.fan@windriver.com>
>>
>> Generally mcstransd works well on MLS enabled system, but if "Reload
>> Translations" triggered, it will fail to translate any MLS level.
>>
>> * Why it fails:
>>
>> Check process_trans() in mcstrans.c:
>>
>>   723 process_trans(char *buffer) {
>>   724         static domain_t *domain;
>>   [snip] ...
>>   784         if (!domain) {
>>   785                 domain = create_domain("Default");
>>
>> While reloading translations, the struct of domain will be destroyed
>> but the static pointer will be kept, it becomes wild and prevents the
>> create_domain() from running; Then invalid domain will be used for
>> initializing hashtable that stores translations data.
>>
>> * Fix to it:
>>
>> Define local *domain to get the struct of domain always be initialized;
>> Use static hashtable to store all translations data.
>>
>> Signed-off-by: Wenzong Fan <wenzong.fan@windriver.com>
>> ---
>>   policycoreutils/mcstrans/src/mcstrans.c | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/policycoreutils/mcstrans/src/mcstrans.c b/policycoreutils/mcstrans/src/mcstrans.c
>> index 4d31857..e8eda32 100644
>> --- a/policycoreutils/mcstrans/src/mcstrans.c
>> +++ b/policycoreutils/mcstrans/src/mcstrans.c
>> @@ -100,11 +100,14 @@ typedef struct base_classification {
>>   	struct base_classification *next;
>>   } base_classification_t;
>>
>> +static context_map_node_t *table_raw_to_trans[N_BUCKETS];
>> +static context_map_node_t *table_trans_to_raw[N_BUCKETS];
>> +
>>   typedef struct domain {
>>   	char *name;
>>
>> -	context_map_node_t *raw_to_trans[N_BUCKETS];
>> -	context_map_node_t *trans_to_raw[N_BUCKETS];
>> +	context_map_node_t **raw_to_trans;
>> +	context_map_node_t **trans_to_raw;
>>
>>   	base_classification_t *base_classifications;
>>   	word_group_t *groups;
>> @@ -643,9 +646,11 @@ add_cache(domain_t *domain, char *raw, char *trans) {
>>   	}
>>
>>   	log_debug(" add_cache (%s,%s)\n", raw, trans);
>> +	domain->raw_to_trans = table_raw_to_trans;
>>   	if (add_to_hashtable(domain->raw_to_trans, map->raw, map) < 0)
>>   		goto err;
>>
>> +	domain->trans_to_raw = table_trans_to_raw;
>>   	if (add_to_hashtable(domain->trans_to_raw, map->trans, map) < 0)
>>   		goto err;
>>
>> @@ -721,7 +726,7 @@ static int read_translations(const char *filename);
>>    */
>>   static int
>>   process_trans(char *buffer) {
>> -	static domain_t *domain;
>> +	domain_t *domain;
>
> Not really familiar with this code, but if you make this change, then
> where is domain initialized prior to first use?

The local domain will be initialized every time while calling the 
process_trans():

784         if (!domain) {
785                 domain = create_domain("Default");

but to be clear, the local "domain" definition should be:

+	domain_t *domain = NULL;

>
>>   	static word_group_t *group;
>>   	static int base_classification;
>>   	static int lineno = 0;
>
> And why is it ok for these to remain static?

Oh, I need more test to verify those features, my testcase is only for 
default "setrans.conf".

I'll update the patch after all test done.

Thanks
Wenzong

>
>
>