| Message ID | 20200303112910.147788-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | selinux: clean up error path in policydb_init() | expand |
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.
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.
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>
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.
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; } /*
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(-)