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

[Stephen Smalley <sds@xxxxxxxxxxxxx>]

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;
> > >        }