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@xxxxxxxxxxxxx] 发送时间: 2019年11月13日 21:13 收件人: kongbaichuan <kongbaichuan@xxxxxxxxxx>; selinux@xxxxxxxxxxxxxxx 主题: 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@xxxxxxxxxxxxx] > 发送时间: 2019年11月12日 23:55 > 收件人: kongbaichuan <kongbaichuan@xxxxxxxxxx>; selinux@xxxxxxxxxxxxxxx > 主题: 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@xxxxxxxxxx> >>> >>> 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@xxxxxxxxxx> >> >> Acked-by: Stephen Smalley <sds@xxxxxxxxxxxxx> > > 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; >>> } >>> >> >
