On Tue, Jul 30, 2019 at 12:48 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> On Mon, Jul 29, 2019 at 4:41 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> >
> > Since roles_init() adds some entries to the role hash table, we need to
> > destroy also its keys/values on error, otherwise we get a memory leak in
> > the error path.
> >
> > To avoid a forward declaration and maintain a sane layout, move all the
> > destroy stuff above policydb_init. No changes are made to the moved code
> > in this patch. Note that this triggers some pre-existing checkpatch.pl
> > warnings - these will be fixed in follow-up patches.
> >
> > Reported-by: syzbot+fee3a14d4cdf92646287@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> > ---
> > security/selinux/ss/policydb.c | 976 +++++++++++++++++----------------
> > 1 file changed, 489 insertions(+), 487 deletions(-)
>
> Hmmm, that is one ugly patch isn't it? If I saw this diff I'm not
> sure I would have suggested what I did, or rather I would have
> suggested something slightly different.
>
> When I ran my quick test when I was looking at your v1 patch, I only
> moved perm_destroy() through ocontext_destroy(), leaving out
> policydb_destroy(), and the diff was much more cleaner[*] (diffstat
> below, includes the actual fix too). Could you try that and see if it
> cleans up your patch?
Yeah, excluding policydb_destroy() from the move is what's needed to
get a nice patch... Actually, what do you think about keeping the
bugfix patch as before (with the forward declaration) and then doing
the moving around in a separate patch (removing the forward
declaration)? Then we keep the patch with the actual fix small, but
still get a clean final result. It would also allow moving
policydb_destroy() up closer to the other destroy functions in another
separate patch (I tried it and both patches end up clean when the move
is split up like this). (I don't have a strong preference for this,
let me know what works best for you.)
>
> security/selinux/ss/policydb.c | 378 +++++++++++++++++-----------------
> 1 file changed, 190 insertions(+), 188 deletions(-)
>
> [*] In this case "cleaner" simply means that the moved lines were not
> interleaved with existing code (just a big block of adds at the top,
> the fix in the middle, and a big block of removals at the bottom).
>
> --
> paul moore
> www.paul-moore.com
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
