[Top][All Lists]

[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: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH 1/6] scripts/coccinelle: add error-use-after-free.cocci
Date: Tue, 31 Mar 2020 16:49:21 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1

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 

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 
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
+// 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

* 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.

index b5c86ec494..ba97cc43fc 100644
@@ -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

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,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]