[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/6] scripts/coccinelle: add error-use-after-free.cocci
From: |
Markus Armbruster |
Subject: |
Re: [PATCH 1/6] scripts/coccinelle: add error-use-after-free.cocci |
Date: |
Tue, 31 Mar 2020 20:38:39 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
Vladimir Sementsov-Ogievskiy <address@hidden> writes:
> 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
For the record, with the globbing replaced, and the pastos fixed:
Reviewed-by: Markus Armbruster <address@hidden>
[PATCH 2/6] block/mirror: fix use after free of local_err, Vladimir Sementsov-Ogievskiy, 2020/03/24
Re: [PATCH 2/6] block/mirror: fix use after free of local_err, Eric Blake, 2020/03/25