* [PATCH] selinux: clean up error path in policydb_init()
@ 2020-03-03 11:29 Ondrej Mosnacek
2020-03-03 19:14 ` Stephen Smalley
0 siblings, 1 reply; 5+ messages in thread
From: Ondrej Mosnacek @ 2020-03-03 11:29 UTC (permalink / raw)
To: selinux, Paul Moore; +Cc: Stephen Smalley, Stephen Smalley
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;
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;
}
/*
--
2.24.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] selinux: clean up error path in policydb_init()
2020-03-03 11:29 [PATCH] selinux: clean up error path in policydb_init() Ondrej Mosnacek
@ 2020-03-03 19:14 ` Stephen Smalley
2020-03-04 9:37 ` Ondrej Mosnacek
0 siblings, 1 reply; 5+ messages in thread
From: Stephen Smalley @ 2020-03-03 19:14 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: SElinux list, Paul Moore, Stephen Smalley
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.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] selinux: clean up error path in policydb_init()
2020-03-03 19:14 ` Stephen Smalley
@ 2020-03-04 9:37 ` Ondrej Mosnacek
2020-03-04 14:29 ` Stephen Smalley
2020-03-05 19:54 ` Paul Moore
0 siblings, 2 replies; 5+ messages in thread
From: Ondrej Mosnacek @ 2020-03-04 9:37 UTC (permalink / raw)
To: Stephen Smalley; +Cc: SElinux list, Paul Moore, Stephen Smalley
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.
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] selinux: clean up error path in policydb_init()
2020-03-04 9:37 ` Ondrej Mosnacek
@ 2020-03-04 14:29 ` Stephen Smalley
2020-03-05 19:54 ` Paul Moore
1 sibling, 0 replies; 5+ messages in thread
From: Stephen Smalley @ 2020-03-04 14:29 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: SElinux list, Paul Moore, Stephen Smalley
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>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] selinux: clean up error path in policydb_init()
2020-03-04 9:37 ` Ondrej Mosnacek
2020-03-04 14:29 ` Stephen Smalley
@ 2020-03-05 19:54 ` Paul Moore
1 sibling, 0 replies; 5+ messages in thread
From: Paul Moore @ 2020-03-05 19:54 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: Stephen Smalley, SElinux list, Stephen Smalley
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.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-03-05 19:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-03 11:29 [PATCH] selinux: clean up error path in policydb_init() Ondrej Mosnacek
2020-03-03 19:14 ` Stephen Smalley
2020-03-04 9:37 ` Ondrej Mosnacek
2020-03-04 14:29 ` Stephen Smalley
2020-03-05 19:54 ` Paul Moore
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).