Re: [PATCH v5 1/2] libsepol,checkpolicy: optimize storage of filename transitions

[Ondrej Mosnacek <omosnace@redhat.com>]

On Tue, Jul 21, 2020 at 8:23 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Sun, Jul 19, 2020 at 6:35 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > In preparation to support a new policy format with a more optimal
> > representation of filename transition rules, this patch applies an
> > equivalent change from kernel commit c3a276111ea2 ("selinux: optimize
> > storage of filename transitions").
> >
> > See the kernel commit's description [1] for the rationale behind this
> > representation. This change doesn't bring any measurable difference of
> > policy build performance (semodule -B) on Fedora.
> >
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git/commit/?id=c3a276111ea2572399281988b3129683e2a6b60b
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
>
> > diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
> > index c6733fa4..01a90438 100644
> > --- a/checkpolicy/policy_define.c
> > +++ b/checkpolicy/policy_define.c
> > @@ -3388,40 +3387,21 @@ int define_filename_trans(void)
> <snip>
> > +                               dup_name = NULL;
> > +                               rc = policydb_filetrans_insert(
> > +                                       policydbp, s+1, t+1, c+1, name,
> > +                                       &dup_name, otype, NULL
> > +                               );
> > +                               free(dup_name);
>
> The dup_name / name_alloc handling seems rather odd.  In every caller
> except one you follow the pattern above, where we set it to NULL, call
> policydb_filetrans_insert, and then free it immediately.  I'm not sure
> why you are doing it this way.

The original intent was to allow a choice between putting the already
allocated string into the key (when the caller function owns it) vs.
allocating a new string (when the caller gets just a read-only
reference to it), while also leaving the string allocated in case such
key already exists (so that the caller can use it in an error message,
as filename_trans_read_one_compat() used to do). However, after
rebasing on top of d27aa22dbeec ("libsepol: drop broken warning on
duplicate filename transitions") such function interface seems to be
no longer optimal... I'll see if I can simplify it.

Thanks for pointing it out.

-- 
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.