selinux: clean up error path in policydb_init()
diff mbox series

Message ID 20200303112910.147788-1-omosnace@redhat.com
State Accepted
Headers show
Series
  • selinux: clean up error path in policydb_init()
Related show

Commit Message

Ondrej Mosnacek March 3, 2020, 11:29 a.m. UTC
Commit e0ac568de1fa ("selinux: reduce the use of hard-coded hash sizes")
moved symtab initialization out of policydb_init(), but left the cleanup
of symtabs from the error path. This patch fixes the oversight.

Suggested-by: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/ss/policydb.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

Comments

Stephen Smalley March 3, 2020, 7:14 p.m. UTC | #1
On Tue, Mar 3, 2020 at 6:29 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> Commit e0ac568de1fa ("selinux: reduce the use of hard-coded hash sizes")
> moved symtab initialization out of policydb_init(), but left the cleanup
> of symtabs from the error path. This patch fixes the oversight.
>
> Suggested-by: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/ss/policydb.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 7739369f5d9a..00edcd216aaa 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -463,36 +463,28 @@ static int rangetr_cmp(struct hashtab *h, const void *k1, const void *k2)
>   */
>  static int policydb_init(struct policydb *p)
>  {
> -       int i, rc;
> +       int rc;
>
>         memset(p, 0, sizeof(*p));
>
>         rc = avtab_init(&p->te_avtab);
>         if (rc)
> -               goto out;
> +               return rc;
>
>         rc = cond_policydb_init(p);
>         if (rc)
> -               goto out;
> +               return rc;

Looks like avtab_init() and cond_policydb_init() can no longer return
errors, merely initialize fields to 0/NULL,
which is already done via the memset above, and are not used anywhere
else so those can go away entirely?

>
>         p->filename_trans = hashtab_create(filenametr_hash, filenametr_cmp,
>                                            (1 << 11));
> -       if (!p->filename_trans) {
> -               rc = -ENOMEM;
> -               goto out;
> -       }
> +       if (!p->filename_trans)
> +               return -ENOMEM;
>
>         ebitmap_init(&p->filename_trans_ttypes);
>         ebitmap_init(&p->policycaps);
>         ebitmap_init(&p->permissive_map);

Technically these aren't needed either but I guess we can leave them
in case ebitmap_init() does more than just memset
at some point.
Ondrej Mosnacek March 4, 2020, 9:37 a.m. UTC | #2
On Tue, Mar 3, 2020 at 8:12 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Tue, Mar 3, 2020 at 6:29 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > Commit e0ac568de1fa ("selinux: reduce the use of hard-coded hash sizes")
> > moved symtab initialization out of policydb_init(), but left the cleanup
> > of symtabs from the error path. This patch fixes the oversight.
> >
> > Suggested-by: Stephen Smalley <sds@tycho.nsa.gov>
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >  security/selinux/ss/policydb.c | 18 +++++-------------
> >  1 file changed, 5 insertions(+), 13 deletions(-)
> >
> > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> > index 7739369f5d9a..00edcd216aaa 100644
> > --- a/security/selinux/ss/policydb.c
> > +++ b/security/selinux/ss/policydb.c
> > @@ -463,36 +463,28 @@ static int rangetr_cmp(struct hashtab *h, const void *k1, const void *k2)
> >   */
> >  static int policydb_init(struct policydb *p)
> >  {
> > -       int i, rc;
> > +       int rc;
> >
> >         memset(p, 0, sizeof(*p));
> >
> >         rc = avtab_init(&p->te_avtab);
> >         if (rc)
> > -               goto out;
> > +               return rc;
> >
> >         rc = cond_policydb_init(p);
> >         if (rc)
> > -               goto out;
> > +               return rc;
>
> Looks like avtab_init() and cond_policydb_init() can no longer return
> errors, merely initialize fields to 0/NULL,
> which is already done via the memset above, and are not used anywhere
> else so those can go away entirely?

OK, but that can be done in a separate patch, right? Do you plan to
send it? Anyway, I'd prefer to keep the *_init() functions for the
sake of abstraction - I'd suggest just changing the return type to
void where possible.
Stephen Smalley March 4, 2020, 2:29 p.m. UTC | #3
On Wed, Mar 4, 2020 at 4:37 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Tue, Mar 3, 2020 at 8:12 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> > On Tue, Mar 3, 2020 at 6:29 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > >
> > > Commit e0ac568de1fa ("selinux: reduce the use of hard-coded hash sizes")
> > > moved symtab initialization out of policydb_init(), but left the cleanup
> > > of symtabs from the error path. This patch fixes the oversight.
> > >
> > > Suggested-by: Stephen Smalley <sds@tycho.nsa.gov>
> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > ---
> > Looks like avtab_init() and cond_policydb_init() can no longer return
> > errors, merely initialize fields to 0/NULL,
> > which is already done via the memset above, and are not used anywhere
> > else so those can go away entirely?
>
> OK, but that can be done in a separate patch, right? Do you plan to
> send it? Anyway, I'd prefer to keep the *_init() functions for the
> sake of abstraction - I'd suggest just changing the return type to
> void where possible.

Fair enough.

Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
Paul Moore March 5, 2020, 7:54 p.m. UTC | #4
On Wed, Mar 4, 2020 at 4:37 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Tue, Mar 3, 2020 at 8:12 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> > On Tue, Mar 3, 2020 at 6:29 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > >
> > > Commit e0ac568de1fa ("selinux: reduce the use of hard-coded hash sizes")
> > > moved symtab initialization out of policydb_init(), but left the cleanup
> > > of symtabs from the error path. This patch fixes the oversight.
> > >
> > > Suggested-by: Stephen Smalley <sds@tycho.nsa.gov>
> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > ---
> > >  security/selinux/ss/policydb.c | 18 +++++-------------
> > >  1 file changed, 5 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> > > index 7739369f5d9a..00edcd216aaa 100644
> > > --- a/security/selinux/ss/policydb.c
> > > +++ b/security/selinux/ss/policydb.c
> > > @@ -463,36 +463,28 @@ static int rangetr_cmp(struct hashtab *h, const void *k1, const void *k2)
> > >   */
> > >  static int policydb_init(struct policydb *p)
> > >  {
> > > -       int i, rc;
> > > +       int rc;
> > >
> > >         memset(p, 0, sizeof(*p));
> > >
> > >         rc = avtab_init(&p->te_avtab);
> > >         if (rc)
> > > -               goto out;
> > > +               return rc;
> > >
> > >         rc = cond_policydb_init(p);
> > >         if (rc)
> > > -               goto out;
> > > +               return rc;
> >
> > Looks like avtab_init() and cond_policydb_init() can no longer return
> > errors, merely initialize fields to 0/NULL,
> > which is already done via the memset above, and are not used anywhere
> > else so those can go away entirely?
>
> OK, but that can be done in a separate patch, right? Do you plan to
> send it? Anyway, I'd prefer to keep the *_init() functions for the
> sake of abstraction - I'd suggest just changing the return type to
> void where possible.

I tend to agree.  Merged into selinux/next.

I'm also not seeing a patch from anyone to change the return type so
I'll put one together now.

Patch
diff mbox series

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 7739369f5d9a..00edcd216aaa 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -463,36 +463,28 @@  static int rangetr_cmp(struct hashtab *h, const void *k1, const void *k2)
  */
 static int policydb_init(struct policydb *p)
 {
-	int i, rc;
+	int rc;
 
 	memset(p, 0, sizeof(*p));
 
 	rc = avtab_init(&p->te_avtab);
 	if (rc)
-		goto out;
+		return rc;
 
 	rc = cond_policydb_init(p);
 	if (rc)
-		goto out;
+		return rc;
 
 	p->filename_trans = hashtab_create(filenametr_hash, filenametr_cmp,
 					   (1 << 11));
-	if (!p->filename_trans) {
-		rc = -ENOMEM;
-		goto out;
-	}
+	if (!p->filename_trans)
+		return -ENOMEM;
 
 	ebitmap_init(&p->filename_trans_ttypes);
 	ebitmap_init(&p->policycaps);
 	ebitmap_init(&p->permissive_map);
 
 	return 0;
-out:
-	for (i = 0; i < SYM_NUM; i++) {
-		hashtab_map(p->symtab[i].table, destroy_f[i], NULL);
-		hashtab_destroy(p->symtab[i].table);
-	}
-	return rc;
 }
 
 /*