Re: should GNU install call matchpathcon by default?

[Stephen Smalley]

On Mon, 2007-11-12 at 14:20 +0100, Jim Meyering wrote:
> Jim Meyering <address@hidden> wrote:
> > Jim Meyering <address@hidden> wrote:
> >> This morning I noticed a flagrant difference in the speed of
> >> "make install" for the just-released gettext-0.17.  It took 12(!)
> >> times longer on a rawhide system than on a usually-slower debian
> >> unstable system. (3min vs. 15s)
> >
> > FYI,
> >
> > Dan Walsh suggested to use
> > matchpathcon_init_prefix (NULL, "/first_component_of_abs_dest/");
> > to limit the number of regular expressions matchpathcon will have to
> > compile.  That works very well, as long as you're not installing into
> > /usr, in which case it's still better than nothing.  When installing
> > into /tmp, the example above takes 21-22 seconds, rather than 180.
> > Much better.  However, installing into /usr/tmp still required about 70
> > seconds, so there's room for improvement.
> 
> I've implemented it like this:
> 
>       install+SELinux: reduce a 12x performance hit to ~1.5x
>       * src/install.c (setdefaultfilecon): Call matchpathcon_init_prefix,
>       to mitigate what would otherwise be a large performance hit due to
>       the use of matchpathcon.
>       Dan Walsh suggested the use of matchpathcon_init_prefix.
>       * gl/lib/se-selinux.in.h (matchpathcon_init_prefix): Define.
> 
> ---
>  gl/lib/se-selinux.in.h |    3 +++
>  src/install.c          |   32 ++++++++++++++++++++++++++++++++
>  3 files changed, 44 insertions(+), 0 deletions(-)
> 
> diff --git a/gl/lib/se-selinux.in.h b/gl/lib/se-selinux.in.h
> index 7bfe4c5..7be1e70 100644
> --- a/gl/lib/se-selinux.in.h
> +++ b/gl/lib/se-selinux.in.h
> @@ -51,4 +51,7 @@ static inline int security_compute_create 
> (security_context_t scon,
>                                          security_class_t tclass,
>                                          security_context_t *newcon)
>    { errno = ENOTSUP; return -1; }
> +static inline int matchpathcon_init_prefix (char const *path,
> +                                         char const *prefix)
> +  { errno = ENOTSUP; return -1; }
>  #endif
> diff --git a/src/install.c b/src/install.c
> index 34f61ff..216715f 100644
> --- a/src/install.c
> +++ b/src/install.c
> @@ -213,6 +213,38 @@ setdefaultfilecon (char const *file)
>    if (lstat (file, &st) != 0)
>      return;
> 
> +  if (IS_ABSOLUTE_FILE_NAME (file))
> +    {
> +      /* Calling matchpathcon_init_prefix (NULL, "/first_component/")
> +      is an optimization to minimize the expense of the following
> +      matchpathcon call.  */
> +      char const *p0;
> +      char const *p = file + 1;
> +      while (ISSLASH (*p))
> +     ++p;
> +
> +      /* Record final leading slash, for when FILE starts with two or more.  
> */
> +      p0 = p - 1;
> +
> +      if (*p)
> +     {
> +       char *prefix;
> +       do
> +         {
> +           ++p;
> +         }
> +       while (*p && !ISSLASH (*p));
> +
> +       prefix = malloc (p - p0 + 2);
> +       if (prefix)
> +         {
> +           stpcpy (stpncpy (prefix, p0, p - p0), "/");
> +           matchpathcon_init_prefix (NULL, prefix);
> +           free (prefix);
> +         }
> +     }
> +    }
> +
>    /* If there's an error determining the context, or it has none,
>       return to allow default context */
>    if ((matchpathcon (file, st.st_mode, &scontext) != 0) ||

This issue came up recently again, see:
https://bugzilla.redhat.com/show_bug.cgi?id=447410

It appears that the patch that was merged into coreutils ends up calling
matchpathcon_init_prefix() for each file being installed rather than
once upon startup, and without calling matchpathcon_fini() to free the
memory allocated by each matchpathcon_init_prefix() call.

That makes it slower than necessary and leaks memory.

See the bug report for the discussion.

Can we get this corrected in the upstream coreutils?  Thanks.

-- 
Stephen Smalley
National Security Agency