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

[Markus Armbruster <armbru@redhat.com>]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 31.03.2020 12:00, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> 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 <vsementsov@virtuozzo.com>
>>> ---
>>>   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?

>>> +
>>> +@ 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.

>>>     GDB stub
>>>   M: Alex Bennée <alex.bennee@linaro.org>
>>