Re: [PATCH 1/6] scripts/coccinelle: add error-use-after-free.cocci

[Vladimir Sementsov-Ogievskiy]

31.03.2020 16:14, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <address@hidden> writes:
>
> > 31.03.2020 12:00, Markus Armbruster wrote:
> > > Vladimir Sementsov-Ogievskiy <address@hidden> writes:
> > >
> > > > Add script to find and fix trivial use-after-free of Error objects.
> > > > How to use:
> > > > spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
> > > >    --macro-file scripts/cocci-macro-file.h --in-place \
> > > >    --no-show-diff ( FILES... | --use-gitgrep . )
> > >
> > > Pasto: you mean scripts/coccinelle/error-use-after-free.cocci.
> > >
> > > --use-gitgrep is just one of several methods.  Any particular reason for
> > > recommending it over the others?
> >
> > :)
> >
> > In my occasional coccinelle learning, every new bit of information wanders me, and I 
> > think "wow! it's tricky/weird/cool (underline whatever applicable), I should note it 
> > somewhere".
> >
> > So, no particular reasons. It's just good thing too use.
> >
> > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > > > ---
> > > >    scripts/coccinelle/error-use-after-free.cocci | 52 +++++++++++++++++++
> > > >    MAINTAINERS                                   |  1 +
> > > >    2 files changed, 53 insertions(+)
> > > >    create mode 100644 scripts/coccinelle/error-use-after-free.cocci
> > > >
> > > > diff --git a/scripts/coccinelle/error-use-after-free.cocci 
> > > > b/scripts/coccinelle/error-use-after-free.cocci
> > > > new file mode 100644
> > > > index 0000000000..7cfa42355b
> > > > --- /dev/null
> > > > +++ b/scripts/coccinelle/error-use-after-free.cocci
> > > > @@ -0,0 +1,52 @@
> > > > +// Find and fix trivial use-after-free of Error objects
> > > > +//
> > > > +// Copyright (c) 2020 Virtuozzo International GmbH.
> > > > +//
> > > > +// This program is free software; you can redistribute it and/or
> > > > +// modify it under the terms of the GNU General Public License as
> > > > +// published by the Free Software Foundation; either version 2 of the
> > > > +// License, or (at your option) any later version.
> > > > +//
> > > > +// This program is distributed in the hope that it will be useful,
> > > > +// but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > > +// GNU General Public License for more details.
> > > > +//
> > > > +// You should have received a copy of the GNU General Public License
> > > > +// along with this program.  If not, see
> > > > +// <http://www.gnu.org/licenses/>.
> > > > +//
> > > > +// How to use:
> > > > +// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
> > > > +//  --macro-file scripts/cocci-macro-file.h --in-place \
> > > > +//  --no-show-diff ( FILES... | --use-gitgrep . )
> > >
> > > Same pasto.
> > >
> > > I doubt including basic spatch instructions with every script is a good
> > > idea.  Better explain it in one place, with proper maintenance.
> > > scripts/coccinelle/README?  We could be a bit more verbose there,
> > > e.g. to clarify required vs. suggested options.
> >
> > Agree, good idea.
>
> I'd like to get your fixes into -rc1, due today.  Possible ways to get
> there:
>
> * You respin with such a README.
>
> * We take the script as is, and move basic spatch instructions to a
>    README at our leisure.  Less stressful, slightly more churn, and we
>    need to remember to actually do it.
>
> I favor the latter.  You?

Me too.

> > > > +
> > > > +@ exists@
> > > > +identifier fn, fn2;
> > > > +expression err;
> > > > +@@
> > > > +
> > > > + fn(...)
> > > > + {
> > > > +     <...
> > > > +(
> > > > +     error_free(err);
> > > > ++    err = NULL;
> > > > +|
> > > > +     error_report_err(err);
> > > > ++    err = NULL;
> > > > +|
> > > > +     error_reportf_err(err, ...);
> > > > ++    err = NULL;
> > > > +|
> > > > +     warn_report_err(err);
> > > > ++    err = NULL;
> > > > +|
> > > > +     warn_reportf_err(err, ...);
> > > > ++    err = NULL;
> > > > +)
> > > > +     ... when != err = NULL
> > > > +         when != exit(...)
> > > > +     fn2(..., err, ...)
> > > > +     ...>
> > > > + }
> > >
> > > This inserts err = NULL after error_free() if there is a path to a
> > > certain kind of use of @err without such an assignment.
> > >
> > > The "when != exit()" part excludes certain "phony" paths.  It's not a
> > > tight check; there are other ways to unconditionally terminate the
> > > process or jump elsewhere behind Coccinelle's back.  Not a problem, the
> > > script is meant to have its output reviewed manually.
> > >
> > > Should we mention the need to review the script's output?
> >
> > I think it's default thing to do.
>
> True.  I just wonder whether we wan to document the difference (assuming
> it exists) between "the output of this script is expected to be good
> (but do review it anyway)" and "this script makes suggestions for you to
> review".  Different levels of confidence in the script, basically.
>
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index b5c86ec494..ba97cc43fc 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -2037,6 +2037,7 @@ F: include/qemu/error-report.h
> > > >    F: qapi/error.json
> > > >    F: util/error.c
> > > >    F: util/qemu-error.c
> > > > +F: scripts/coccinelle/*err*.cocci
> > >
> > > Silently captures existing scripts in addition to this new one.
> > > Tolerable.  The globbing looks rather brittle, though.
> >
> > hmm, may be better to rename them all to "error-*.cocci"
>
> Would permit reasonably robust globbing.  Fine with me, but requires a
> respin.
>
> I'm also fine with enumerating the scripts here one by one.  That I could do
> myself without a respin.

no objections

> > > >      GDB stub
> > > >    M: Alex Bennée <address@hidden>


--
Best regards,
Vladimir