Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> writes: > 31.03.2020 16:14, Markus Armbruster wrote: >> Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> writes: >> >>> 31.03.2020 12:00, Markus Armbruster wrote: >>>> Vladimir Sementsov-Ogievskiy <vsement...@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 <vsement...@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? > > 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 <arm...@redhat.com>
