[PATCH] restorecond: Fix redundant console log output error

[PATCH] restorecond: Fix redundant console log output error

[Baichuan Kong]

From: kong baichuan <kongbaichuan@huawei.com>

When starting restorecond without any option the following redundant
console log is outputed:

/dev/log 100.0%
/var/volatile/run/syslogd.pid 100.0%
...

This is caused by two global variables of same name r_opts. When
executes r_opts = opts in restore_init(), it originally intends
to assign the address of struct r_opts in "restorecond.c" to the
pointer *r_opts in "restore.c".

However, the address is assigned to the struct r_opts and covers
the value of low eight bytes in it. That causes unexpected value
of member varibale 'nochange' and 'verbose' in struct r_opts, thus
affects value of 'restorecon_flags' and executes unexpected operations
when restorecon the files such as the redundant console log output or
file label nochange.

Signed-off-by: kong baichuan <kongbaichuan@huawei.com>
---
 restorecond/restore.c | 40 ++++++++++++++++++----------------------
 1 file changed, 18 insertions(+), 22 deletions(-)

diff --git a/restorecond/restore.c b/restorecond/restore.c
index f6e30001..b93b5fdb 100644
--- a/restorecond/restore.c
+++ b/restorecond/restore.c
@@ -12,39 +12,36 @@
 char **exclude_list;
 int exclude_count;
 
-struct restore_opts *r_opts;
-
 void restore_init(struct restore_opts *opts)
 {
 	int rc;
 
-	r_opts = opts;
 	struct selinux_opt selinux_opts[] = {
-		{ SELABEL_OPT_VALIDATE, r_opts->selabel_opt_validate },
-		{ SELABEL_OPT_PATH, r_opts->selabel_opt_path },
-		{ SELABEL_OPT_DIGEST, r_opts->selabel_opt_digest }
+		{ SELABEL_OPT_VALIDATE, opts->selabel_opt_validate },
+		{ SELABEL_OPT_PATH, opts->selabel_opt_path },
+		{ SELABEL_OPT_DIGEST, opts->selabel_opt_digest }
 	};
 
-	r_opts->hnd = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3);
-	if (!r_opts->hnd) {
-		perror(r_opts->selabel_opt_path);
+	opts->hnd = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3);
+	if (!opts->hnd) {
+		perror(opts->selabel_opt_path);
 		exit(1);
 	}
 
-	r_opts->restorecon_flags = 0;
-	r_opts->restorecon_flags = r_opts->nochange | r_opts->verbose |
-			   r_opts->progress | r_opts->set_specctx  |
-			   r_opts->add_assoc | r_opts->ignore_digest |
-			   r_opts->recurse | r_opts->userealpath |
-			   r_opts->xdev | r_opts->abort_on_error |
-			   r_opts->syslog_changes | r_opts->log_matches |
-			   r_opts->ignore_noent | r_opts->ignore_mounts;
+	opts->restorecon_flags = 0;
+	opts->restorecon_flags = opts->nochange | opts->verbose |
+			   opts->progress | opts->set_specctx  |
+			   opts->add_assoc | opts->ignore_digest |
+			   opts->recurse | opts->userealpath |
+			   opts->xdev | opts->abort_on_error |
+			   opts->syslog_changes | opts->log_matches |
+			   opts->ignore_noent | opts->ignore_mounts;
 
 	/* Use setfiles, restorecon and restorecond own handles */
-	selinux_restorecon_set_sehandle(r_opts->hnd);
+	selinux_restorecon_set_sehandle(opts->hnd);
 
-	if (r_opts->rootpath) {
-		rc = selinux_restorecon_set_alt_rootpath(r_opts->rootpath);
+	if (opts->rootpath) {
+		rc = selinux_restorecon_set_alt_rootpath(opts->rootpath);
 		if (rc) {
 			fprintf(stderr,
 				"selinux_restorecon_set_alt_rootpath error: %s.\n",
@@ -75,7 +72,6 @@ int process_glob(char *name, struct restore_opts *opts)
 	size_t i = 0;
 	int len, rc, errors;
 
-	r_opts = opts;
 	memset(&globbuf, 0, sizeof(globbuf));
 
 	errors = glob(name, GLOB_TILDE | GLOB_PERIOD |
@@ -90,7 +86,7 @@ int process_glob(char *name, struct restore_opts *opts)
 		if (len > 0 && strcmp(&globbuf.gl_pathv[i][len], "/..") == 0)
 			continue;
 		rc = selinux_restorecon(globbuf.gl_pathv[i],
-					r_opts->restorecon_flags);
+					opts->restorecon_flags);
 		if (rc < 0)
 			errors = rc;
 	}
-- 
2.23.0.windows.1

Re: [PATCH] restorecond: Fix redundant console log output error

[Stephen Smalley]

On 11/11/19 8:23 PM, Baichuan Kong wrote:
> From: kong baichuan <kongbaichuan@huawei.com>
> 
> When starting restorecond without any option the following redundant
> console log is outputed:
> 
> /dev/log 100.0%
> /var/volatile/run/syslogd.pid 100.0%
> ...
> 
> This is caused by two global variables of same name r_opts. When
> executes r_opts = opts in restore_init(), it originally intends
> to assign the address of struct r_opts in "restorecond.c" to the
> pointer *r_opts in "restore.c".
> 
> However, the address is assigned to the struct r_opts and covers
> the value of low eight bytes in it. That causes unexpected value
> of member varibale 'nochange' and 'verbose' in struct r_opts, thus
> affects value of 'restorecon_flags' and executes unexpected operations
> when restorecon the files such as the redundant console log output or
> file label nochange.
> 
> Signed-off-by: kong baichuan <kongbaichuan@huawei.com>

Acked-by: Stephen Smalley <sds@tycho.nsa.gov>

> ---
>   restorecond/restore.c | 40 ++++++++++++++++++----------------------
>   1 file changed, 18 insertions(+), 22 deletions(-)
> 
> diff --git a/restorecond/restore.c b/restorecond/restore.c
> index f6e30001..b93b5fdb 100644
> --- a/restorecond/restore.c
> +++ b/restorecond/restore.c
> @@ -12,39 +12,36 @@
>   char **exclude_list;
>   int exclude_count;
>   
> -struct restore_opts *r_opts;
> -
>   void restore_init(struct restore_opts *opts)
>   {
>   	int rc;
>   
> -	r_opts = opts;
>   	struct selinux_opt selinux_opts[] = {
> -		{ SELABEL_OPT_VALIDATE, r_opts->selabel_opt_validate },
> -		{ SELABEL_OPT_PATH, r_opts->selabel_opt_path },
> -		{ SELABEL_OPT_DIGEST, r_opts->selabel_opt_digest }
> +		{ SELABEL_OPT_VALIDATE, opts->selabel_opt_validate },
> +		{ SELABEL_OPT_PATH, opts->selabel_opt_path },
> +		{ SELABEL_OPT_DIGEST, opts->selabel_opt_digest }
>   	};
>   
> -	r_opts->hnd = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3);
> -	if (!r_opts->hnd) {
> -		perror(r_opts->selabel_opt_path);
> +	opts->hnd = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3);
> +	if (!opts->hnd) {
> +		perror(opts->selabel_opt_path);
>   		exit(1);
>   	}
>   
> -	r_opts->restorecon_flags = 0;
> -	r_opts->restorecon_flags = r_opts->nochange | r_opts->verbose |
> -			   r_opts->progress | r_opts->set_specctx  |
> -			   r_opts->add_assoc | r_opts->ignore_digest |
> -			   r_opts->recurse | r_opts->userealpath |
> -			   r_opts->xdev | r_opts->abort_on_error |
> -			   r_opts->syslog_changes | r_opts->log_matches |
> -			   r_opts->ignore_noent | r_opts->ignore_mounts;
> +	opts->restorecon_flags = 0;
> +	opts->restorecon_flags = opts->nochange | opts->verbose |
> +			   opts->progress | opts->set_specctx  |
> +			   opts->add_assoc | opts->ignore_digest |
> +			   opts->recurse | opts->userealpath |
> +			   opts->xdev | opts->abort_on_error |
> +			   opts->syslog_changes | opts->log_matches |
> +			   opts->ignore_noent | opts->ignore_mounts;
>   
>   	/* Use setfiles, restorecon and restorecond own handles */
> -	selinux_restorecon_set_sehandle(r_opts->hnd);
> +	selinux_restorecon_set_sehandle(opts->hnd);
>   
> -	if (r_opts->rootpath) {
> -		rc = selinux_restorecon_set_alt_rootpath(r_opts->rootpath);
> +	if (opts->rootpath) {
> +		rc = selinux_restorecon_set_alt_rootpath(opts->rootpath);
>   		if (rc) {
>   			fprintf(stderr,
>   				"selinux_restorecon_set_alt_rootpath error: %s.\n",
> @@ -75,7 +72,6 @@ int process_glob(char *name, struct restore_opts *opts)
>   	size_t i = 0;
>   	int len, rc, errors;
>   
> -	r_opts = opts;
>   	memset(&globbuf, 0, sizeof(globbuf));
>   
>   	errors = glob(name, GLOB_TILDE | GLOB_PERIOD |
> @@ -90,7 +86,7 @@ int process_glob(char *name, struct restore_opts *opts)
>   		if (len > 0 && strcmp(&globbuf.gl_pathv[i][len], "/..") == 0)
>   			continue;
>   		rc = selinux_restorecon(globbuf.gl_pathv[i],
> -					r_opts->restorecon_flags);
> +					opts->restorecon_flags);
>   		if (rc < 0)
>   			errors = rc;
>   	}
> 

Re: [PATCH] restorecond: Fix redundant console log output error

[Stephen Smalley]

On 11/12/19 10:47 AM, Stephen Smalley wrote:
> On 11/11/19 8:23 PM, Baichuan Kong wrote:
>> From: kong baichuan <kongbaichuan@huawei.com>
>>
>> When starting restorecond without any option the following redundant
>> console log is outputed:
>>
>> /dev/log 100.0%
>> /var/volatile/run/syslogd.pid 100.0%
>> ...
>>
>> This is caused by two global variables of same name r_opts. When
>> executes r_opts = opts in restore_init(), it originally intends
>> to assign the address of struct r_opts in "restorecond.c" to the
>> pointer *r_opts in "restore.c".
>>
>> However, the address is assigned to the struct r_opts and covers
>> the value of low eight bytes in it. That causes unexpected value
>> of member varibale 'nochange' and 'verbose' in struct r_opts, thus
>> affects value of 'restorecon_flags' and executes unexpected operations
>> when restorecon the files such as the redundant console log output or
>> file label nochange.
>>
>> Signed-off-by: kong baichuan <kongbaichuan@huawei.com>
> 
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>

NB restore.c in restorecond was copied from policycoreutils/setfiles, 
which shares this same pattern, except that the separate r_opts 
declaration in setfiles.c is static.  We should likely fix it for 
setfiles as well.

> 
>> ---
>>   restorecond/restore.c | 40 ++++++++++++++++++----------------------
>>   1 file changed, 18 insertions(+), 22 deletions(-)
>>
>> diff --git a/restorecond/restore.c b/restorecond/restore.c
>> index f6e30001..b93b5fdb 100644
>> --- a/restorecond/restore.c
>> +++ b/restorecond/restore.c
>> @@ -12,39 +12,36 @@
>>   char **exclude_list;
>>   int exclude_count;
>> -struct restore_opts *r_opts;
>> -
>>   void restore_init(struct restore_opts *opts)
>>   {
>>       int rc;
>> -    r_opts = opts;
>>       struct selinux_opt selinux_opts[] = {
>> -        { SELABEL_OPT_VALIDATE, r_opts->selabel_opt_validate },
>> -        { SELABEL_OPT_PATH, r_opts->selabel_opt_path },
>> -        { SELABEL_OPT_DIGEST, r_opts->selabel_opt_digest }
>> +        { SELABEL_OPT_VALIDATE, opts->selabel_opt_validate },
>> +        { SELABEL_OPT_PATH, opts->selabel_opt_path },
>> +        { SELABEL_OPT_DIGEST, opts->selabel_opt_digest }
>>       };
>> -    r_opts->hnd = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3);
>> -    if (!r_opts->hnd) {
>> -        perror(r_opts->selabel_opt_path);
>> +    opts->hnd = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3);
>> +    if (!opts->hnd) {
>> +        perror(opts->selabel_opt_path);
>>           exit(1);
>>       }
>> -    r_opts->restorecon_flags = 0;
>> -    r_opts->restorecon_flags = r_opts->nochange | r_opts->verbose |
>> -               r_opts->progress | r_opts->set_specctx  |
>> -               r_opts->add_assoc | r_opts->ignore_digest |
>> -               r_opts->recurse | r_opts->userealpath |
>> -               r_opts->xdev | r_opts->abort_on_error |
>> -               r_opts->syslog_changes | r_opts->log_matches |
>> -               r_opts->ignore_noent | r_opts->ignore_mounts;
>> +    opts->restorecon_flags = 0;
>> +    opts->restorecon_flags = opts->nochange | opts->verbose |
>> +               opts->progress | opts->set_specctx  |
>> +               opts->add_assoc | opts->ignore_digest |
>> +               opts->recurse | opts->userealpath |
>> +               opts->xdev | opts->abort_on_error |
>> +               opts->syslog_changes | opts->log_matches |
>> +               opts->ignore_noent | opts->ignore_mounts;
>>       /* Use setfiles, restorecon and restorecond own handles */
>> -    selinux_restorecon_set_sehandle(r_opts->hnd);
>> +    selinux_restorecon_set_sehandle(opts->hnd);
>> -    if (r_opts->rootpath) {
>> -        rc = selinux_restorecon_set_alt_rootpath(r_opts->rootpath);
>> +    if (opts->rootpath) {
>> +        rc = selinux_restorecon_set_alt_rootpath(opts->rootpath);
>>           if (rc) {
>>               fprintf(stderr,
>>                   "selinux_restorecon_set_alt_rootpath error: %s.\n",
>> @@ -75,7 +72,6 @@ int process_glob(char *name, struct restore_opts *opts)
>>       size_t i = 0;
>>       int len, rc, errors;
>> -    r_opts = opts;
>>       memset(&globbuf, 0, sizeof(globbuf));
>>       errors = glob(name, GLOB_TILDE | GLOB_PERIOD |
>> @@ -90,7 +86,7 @@ int process_glob(char *name, struct restore_opts *opts)
>>           if (len > 0 && strcmp(&globbuf.gl_pathv[i][len], "/..") == 0)
>>               continue;
>>           rc = selinux_restorecon(globbuf.gl_pathv[i],
>> -                    r_opts->restorecon_flags);
>> +                    opts->restorecon_flags);
>>           if (rc < 0)
>>               errors = rc;
>>       }
>>
> 

答复: [PATCH] restorecond: Fix redundant console log output error

[kongbaichuan]

The situation in policycoreutils/setfiles is different with restorecond. 
Two same-name varibales r_opts in policycoreutils/setfiles is not effected with each other, 
so it is not necessary to fix. 


-----邮件原件-----
发件人: Stephen Smalley [mailto:sds@tycho.nsa.gov] 
发送时间: 2019年11月12日 23:55
收件人: kongbaichuan <kongbaichuan@huawei.com>; selinux@vger.kernel.org
主题: Re: [PATCH] restorecond: Fix redundant console log output error

On 11/12/19 10:47 AM, Stephen Smalley wrote:
> On 11/11/19 8:23 PM, Baichuan Kong wrote:
>> From: kong baichuan <kongbaichuan@huawei.com>
>>
>> When starting restorecond without any option the following redundant 
>> console log is outputed:
>>
>> /dev/log 100.0%
>> /var/volatile/run/syslogd.pid 100.0%
>> ...
>>
>> This is caused by two global variables of same name r_opts. When 
>> executes r_opts = opts in restore_init(), it originally intends to 
>> assign the address of struct r_opts in "restorecond.c" to the pointer 
>> *r_opts in "restore.c".
>>
>> However, the address is assigned to the struct r_opts and covers the 
>> value of low eight bytes in it. That causes unexpected value of 
>> member varibale 'nochange' and 'verbose' in struct r_opts, thus 
>> affects value of 'restorecon_flags' and executes unexpected 
>> operations when restorecon the files such as the redundant console 
>> log output or file label nochange.
>>
>> Signed-off-by: kong baichuan <kongbaichuan@huawei.com>
> 
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>

NB restore.c in restorecond was copied from policycoreutils/setfiles, which shares this same pattern, except that the separate r_opts declaration in setfiles.c is static.  We should likely fix it for setfiles as well.

> 
>> ---
>>   restorecond/restore.c | 40 ++++++++++++++++++----------------------
>>   1 file changed, 18 insertions(+), 22 deletions(-)
>>
>> diff --git a/restorecond/restore.c b/restorecond/restore.c
>> index f6e30001..b93b5fdb 100644
>> --- a/restorecond/restore.c
>> +++ b/restorecond/restore.c
>> @@ -12,39 +12,36 @@
>>   char **exclude_list;
>>   int exclude_count;
>> -struct restore_opts *r_opts;
>> -
>>   void restore_init(struct restore_opts *opts)
>>   {
>>       int rc;
>> -    r_opts = opts;
>>       struct selinux_opt selinux_opts[] = {
>> -        { SELABEL_OPT_VALIDATE, r_opts->selabel_opt_validate },
>> -        { SELABEL_OPT_PATH, r_opts->selabel_opt_path },
>> -        { SELABEL_OPT_DIGEST, r_opts->selabel_opt_digest }
>> +        { SELABEL_OPT_VALIDATE, opts->selabel_opt_validate },
>> +        { SELABEL_OPT_PATH, opts->selabel_opt_path },
>> +        { SELABEL_OPT_DIGEST, opts->selabel_opt_digest }
>>       };
>> -    r_opts->hnd = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3);
>> -    if (!r_opts->hnd) {
>> -        perror(r_opts->selabel_opt_path);
>> +    opts->hnd = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3);
>> +    if (!opts->hnd) {
>> +        perror(opts->selabel_opt_path);
>>           exit(1);
>>       }
>> -    r_opts->restorecon_flags = 0;
>> -    r_opts->restorecon_flags = r_opts->nochange | r_opts->verbose |
>> -               r_opts->progress | r_opts->set_specctx  |
>> -               r_opts->add_assoc | r_opts->ignore_digest |
>> -               r_opts->recurse | r_opts->userealpath |
>> -               r_opts->xdev | r_opts->abort_on_error |
>> -               r_opts->syslog_changes | r_opts->log_matches |
>> -               r_opts->ignore_noent | r_opts->ignore_mounts;
>> +    opts->restorecon_flags = 0;
>> +    opts->restorecon_flags = opts->nochange | opts->verbose |
>> +               opts->progress | opts->set_specctx  |
>> +               opts->add_assoc | opts->ignore_digest |
>> +               opts->recurse | opts->userealpath |
>> +               opts->xdev | opts->abort_on_error |
>> +               opts->syslog_changes | opts->log_matches |
>> +               opts->ignore_noent | opts->ignore_mounts;
>>       /* Use setfiles, restorecon and restorecond own handles */
>> -    selinux_restorecon_set_sehandle(r_opts->hnd);
>> +    selinux_restorecon_set_sehandle(opts->hnd);
>> -    if (r_opts->rootpath) {
>> -        rc = selinux_restorecon_set_alt_rootpath(r_opts->rootpath);
>> +    if (opts->rootpath) {
>> +        rc = selinux_restorecon_set_alt_rootpath(opts->rootpath);
>>           if (rc) {
>>               fprintf(stderr,
>>                   "selinux_restorecon_set_alt_rootpath error: %s.\n",
>> @@ -75,7 +72,6 @@ int process_glob(char *name, struct restore_opts *opts)
>>       size_t i = 0;
>>       int len, rc, errors;
>> -    r_opts = opts;
>>       memset(&globbuf, 0, sizeof(globbuf));
>>       errors = glob(name, GLOB_TILDE | GLOB_PERIOD |
>> @@ -90,7 +86,7 @@ int process_glob(char *name, struct restore_opts *opts)
>>           if (len > 0 && strcmp(&globbuf.gl_pathv[i][len], "/..") == 0)
>>               continue;
>>           rc = selinux_restorecon(globbuf.gl_pathv[i],
>> -                    r_opts->restorecon_flags);
>> +                    opts->restorecon_flags);
>>           if (rc < 0)
>>               errors = rc;
>>       }
>>
> 

Re: [PATCH] restorecond: Fix redundant console log output error

[Stephen Smalley]

On 11/13/19 1:36 AM, kongbaichuan wrote:
> The situation in policycoreutils/setfiles is different with restorecond.
> Two same-name varibales r_opts in policycoreutils/setfiles is not effected with each other,
> so it is not necessary to fix.

I understand it isn't a problem in practice right now, but it is a 
maintainability issue; it can create confusion in the future and if 
someone were to ever change the one instance to be non-static in order 
to access it from another source file, we would have exactly the same 
bug.  Hence, I would favor fixing it now.

> 
> 
> -----邮件原件-----
> 发件人: Stephen Smalley [mailto:sds@tycho.nsa.gov]
> 发送时间: 2019年11月12日 23:55
> 收件人: kongbaichuan <kongbaichuan@huawei.com>; selinux@vger.kernel.org
> 主题: Re: [PATCH] restorecond: Fix redundant console log output error
> 
> On 11/12/19 10:47 AM, Stephen Smalley wrote:
>> On 11/11/19 8:23 PM, Baichuan Kong wrote:
>>> From: kong baichuan <kongbaichuan@huawei.com>
>>>
>>> When starting restorecond without any option the following redundant
>>> console log is outputed:
>>>
>>> /dev/log 100.0%
>>> /var/volatile/run/syslogd.pid 100.0%
>>> ...
>>>
>>> This is caused by two global variables of same name r_opts. When
>>> executes r_opts = opts in restore_init(), it originally intends to
>>> assign the address of struct r_opts in "restorecond.c" to the pointer
>>> *r_opts in "restore.c".
>>>
>>> However, the address is assigned to the struct r_opts and covers the
>>> value of low eight bytes in it. That causes unexpected value of
>>> member varibale 'nochange' and 'verbose' in struct r_opts, thus
>>> affects value of 'restorecon_flags' and executes unexpected
>>> operations when restorecon the files such as the redundant console
>>> log output or file label nochange.
>>>
>>> Signed-off-by: kong baichuan <kongbaichuan@huawei.com>
>>
>> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> 
> NB restore.c in restorecond was copied from policycoreutils/setfiles, which shares this same pattern, except that the separate r_opts declaration in setfiles.c is static.  We should likely fix it for setfiles as well.
> 
>>
>>> ---
>>>    restorecond/restore.c | 40 ++++++++++++++++++----------------------
>>>    1 file changed, 18 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/restorecond/restore.c b/restorecond/restore.c
>>> index f6e30001..b93b5fdb 100644
>>> --- a/restorecond/restore.c
>>> +++ b/restorecond/restore.c
>>> @@ -12,39 +12,36 @@
>>>    char **exclude_list;
>>>    int exclude_count;
>>> -struct restore_opts *r_opts;
>>> -
>>>    void restore_init(struct restore_opts *opts)
>>>    {
>>>        int rc;
>>> -    r_opts = opts;
>>>        struct selinux_opt selinux_opts[] = {
>>> -        { SELABEL_OPT_VALIDATE, r_opts->selabel_opt_validate },
>>> -        { SELABEL_OPT_PATH, r_opts->selabel_opt_path },
>>> -        { SELABEL_OPT_DIGEST, r_opts->selabel_opt_digest }
>>> +        { SELABEL_OPT_VALIDATE, opts->selabel_opt_validate },
>>> +        { SELABEL_OPT_PATH, opts->selabel_opt_path },
>>> +        { SELABEL_OPT_DIGEST, opts->selabel_opt_digest }
>>>        };
>>> -    r_opts->hnd = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3);
>>> -    if (!r_opts->hnd) {
>>> -        perror(r_opts->selabel_opt_path);
>>> +    opts->hnd = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3);
>>> +    if (!opts->hnd) {
>>> +        perror(opts->selabel_opt_path);
>>>            exit(1);
>>>        }
>>> -    r_opts->restorecon_flags = 0;
>>> -    r_opts->restorecon_flags = r_opts->nochange | r_opts->verbose |
>>> -               r_opts->progress | r_opts->set_specctx  |
>>> -               r_opts->add_assoc | r_opts->ignore_digest |
>>> -               r_opts->recurse | r_opts->userealpath |
>>> -               r_opts->xdev | r_opts->abort_on_error |
>>> -               r_opts->syslog_changes | r_opts->log_matches |
>>> -               r_opts->ignore_noent | r_opts->ignore_mounts;
>>> +    opts->restorecon_flags = 0;
>>> +    opts->restorecon_flags = opts->nochange | opts->verbose |
>>> +               opts->progress | opts->set_specctx  |
>>> +               opts->add_assoc | opts->ignore_digest |
>>> +               opts->recurse | opts->userealpath |
>>> +               opts->xdev | opts->abort_on_error |
>>> +               opts->syslog_changes | opts->log_matches |
>>> +               opts->ignore_noent | opts->ignore_mounts;
>>>        /* Use setfiles, restorecon and restorecond own handles */
>>> -    selinux_restorecon_set_sehandle(r_opts->hnd);
>>> +    selinux_restorecon_set_sehandle(opts->hnd);
>>> -    if (r_opts->rootpath) {
>>> -        rc = selinux_restorecon_set_alt_rootpath(r_opts->rootpath);
>>> +    if (opts->rootpath) {
>>> +        rc = selinux_restorecon_set_alt_rootpath(opts->rootpath);
>>>            if (rc) {
>>>                fprintf(stderr,
>>>                    "selinux_restorecon_set_alt_rootpath error: %s.\n",
>>> @@ -75,7 +72,6 @@ int process_glob(char *name, struct restore_opts *opts)
>>>        size_t i = 0;
>>>        int len, rc, errors;
>>> -    r_opts = opts;
>>>        memset(&globbuf, 0, sizeof(globbuf));
>>>        errors = glob(name, GLOB_TILDE | GLOB_PERIOD |
>>> @@ -90,7 +86,7 @@ int process_glob(char *name, struct restore_opts *opts)
>>>            if (len > 0 && strcmp(&globbuf.gl_pathv[i][len], "/..") == 0)
>>>                continue;
>>>            rc = selinux_restorecon(globbuf.gl_pathv[i],
>>> -                    r_opts->restorecon_flags);
>>> +                    opts->restorecon_flags);
>>>            if (rc < 0)
>>>                errors = rc;
>>>        }
>>>
>>
> 

答复: [PATCH] restorecond: Fix redundant console log output error

[kongbaichuan]

Thanks for your advice, I understand what you mean and you are right. The new patch is resent
to the mailing list, which fixed policycoreutils/setfiles, please review it. Tks!

-----邮件原件-----
发件人: Stephen Smalley [mailto:sds@tycho.nsa.gov] 
发送时间: 2019年11月13日 21:13
收件人: kongbaichuan <kongbaichuan@huawei.com>; selinux@vger.kernel.org
主题: Re: [PATCH] restorecond: Fix redundant console log output error

On 11/13/19 1:36 AM, kongbaichuan wrote:
> The situation in policycoreutils/setfiles is different with restorecond.
> Two same-name varibales r_opts in policycoreutils/setfiles is not 
> effected with each other, so it is not necessary to fix.

I understand it isn't a problem in practice right now, but it is a maintainability issue; it can create confusion in the future and if someone were to ever change the one instance to be non-static in order to access it from another source file, we would have exactly the same bug.  Hence, I would favor fixing it now.

> 
> 
> -----邮件原件-----
> 发件人: Stephen Smalley [mailto:sds@tycho.nsa.gov]
> 发送时间: 2019年11月12日 23:55
> 收件人: kongbaichuan <kongbaichuan@huawei.com>; selinux@vger.kernel.org
> 主题: Re: [PATCH] restorecond: Fix redundant console log output error
> 
> On 11/12/19 10:47 AM, Stephen Smalley wrote:
>> On 11/11/19 8:23 PM, Baichuan Kong wrote:
>>> From: kong baichuan <kongbaichuan@huawei.com>
>>>
>>> When starting restorecond without any option the following redundant 
>>> console log is outputed:
>>>
>>> /dev/log 100.0%
>>> /var/volatile/run/syslogd.pid 100.0% ...
>>>
>>> This is caused by two global variables of same name r_opts. When 
>>> executes r_opts = opts in restore_init(), it originally intends to 
>>> assign the address of struct r_opts in "restorecond.c" to the 
>>> pointer *r_opts in "restore.c".
>>>
>>> However, the address is assigned to the struct r_opts and covers the 
>>> value of low eight bytes in it. That causes unexpected value of 
>>> member varibale 'nochange' and 'verbose' in struct r_opts, thus 
>>> affects value of 'restorecon_flags' and executes unexpected 
>>> operations when restorecon the files such as the redundant console 
>>> log output or file label nochange.
>>>
>>> Signed-off-by: kong baichuan <kongbaichuan@huawei.com>
>>
>> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> 
> NB restore.c in restorecond was copied from policycoreutils/setfiles, which shares this same pattern, except that the separate r_opts declaration in setfiles.c is static.  We should likely fix it for setfiles as well.
> 
>>
>>> ---
>>>    restorecond/restore.c | 40 
>>> ++++++++++++++++++----------------------
>>>    1 file changed, 18 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/restorecond/restore.c b/restorecond/restore.c index 
>>> f6e30001..b93b5fdb 100644
>>> --- a/restorecond/restore.c
>>> +++ b/restorecond/restore.c
>>> @@ -12,39 +12,36 @@
>>>    char **exclude_list;
>>>    int exclude_count;
>>> -struct restore_opts *r_opts;
>>> -
>>>    void restore_init(struct restore_opts *opts)
>>>    {
>>>        int rc;
>>> -    r_opts = opts;
>>>        struct selinux_opt selinux_opts[] = {
>>> -        { SELABEL_OPT_VALIDATE, r_opts->selabel_opt_validate },
>>> -        { SELABEL_OPT_PATH, r_opts->selabel_opt_path },
>>> -        { SELABEL_OPT_DIGEST, r_opts->selabel_opt_digest }
>>> +        { SELABEL_OPT_VALIDATE, opts->selabel_opt_validate },
>>> +        { SELABEL_OPT_PATH, opts->selabel_opt_path },
>>> +        { SELABEL_OPT_DIGEST, opts->selabel_opt_digest }
>>>        };
>>> -    r_opts->hnd = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3);
>>> -    if (!r_opts->hnd) {
>>> -        perror(r_opts->selabel_opt_path);
>>> +    opts->hnd = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3);
>>> +    if (!opts->hnd) {
>>> +        perror(opts->selabel_opt_path);
>>>            exit(1);
>>>        }
>>> -    r_opts->restorecon_flags = 0;
>>> -    r_opts->restorecon_flags = r_opts->nochange | r_opts->verbose |
>>> -               r_opts->progress | r_opts->set_specctx  |
>>> -               r_opts->add_assoc | r_opts->ignore_digest |
>>> -               r_opts->recurse | r_opts->userealpath |
>>> -               r_opts->xdev | r_opts->abort_on_error |
>>> -               r_opts->syslog_changes | r_opts->log_matches |
>>> -               r_opts->ignore_noent | r_opts->ignore_mounts;
>>> +    opts->restorecon_flags = 0;
>>> +    opts->restorecon_flags = opts->nochange | opts->verbose |
>>> +               opts->progress | opts->set_specctx  |
>>> +               opts->add_assoc | opts->ignore_digest |
>>> +               opts->recurse | opts->userealpath |
>>> +               opts->xdev | opts->abort_on_error |
>>> +               opts->syslog_changes | opts->log_matches |
>>> +               opts->ignore_noent | opts->ignore_mounts;
>>>        /* Use setfiles, restorecon and restorecond own handles */
>>> -    selinux_restorecon_set_sehandle(r_opts->hnd);
>>> +    selinux_restorecon_set_sehandle(opts->hnd);
>>> -    if (r_opts->rootpath) {
>>> -        rc = selinux_restorecon_set_alt_rootpath(r_opts->rootpath);
>>> +    if (opts->rootpath) {
>>> +        rc = selinux_restorecon_set_alt_rootpath(opts->rootpath);
>>>            if (rc) {
>>>                fprintf(stderr,
>>>                    "selinux_restorecon_set_alt_rootpath error: 
>>> %s.\n", @@ -75,7 +72,6 @@ int process_glob(char *name, struct 
>>> restore_opts *opts)
>>>        size_t i = 0;
>>>        int len, rc, errors;
>>> -    r_opts = opts;
>>>        memset(&globbuf, 0, sizeof(globbuf));
>>>        errors = glob(name, GLOB_TILDE | GLOB_PERIOD | @@ -90,7 +86,7 
>>> @@ int process_glob(char *name, struct restore_opts *opts)
>>>            if (len > 0 && strcmp(&globbuf.gl_pathv[i][len], "/..") 
>>> == 0)
>>>                continue;
>>>            rc = selinux_restorecon(globbuf.gl_pathv[i],
>>> -                    r_opts->restorecon_flags);
>>> +                    opts->restorecon_flags);
>>>            if (rc < 0)
>>>                errors = rc;
>>>        }
>>>
>>
> 

答复: [PATCH] restorecond: Fix redundant console log output error

[kongbaichuan]

linux_suse_sp4_work:/tmp # readelf -s setfiles | grep r_opts
    53: 0000000000014020   120 OBJECT  LOCAL  DEFAULT   22 r_opts
   114: 00000000000140c0     8 OBJECT  GLOBAL DEFAULT   22 r_opts
linux_suse_sp4_work:/tmp # readelf -s restorecond | grep r_opts
   101: 00000000000060c0   128 OBJECT  GLOBAL DEFAULT   23 r_opts

This is the entries of r_opts in symbol table of setfiles and restorecond. There is one r_opts is 
coverd by another in restorecond, but it not happened to setfiles.