[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 12:12:22 +0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 |
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.
+
+@ 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.
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"
GDB stub
M: Alex Bennée <address@hidden>
--
Best regards,
Vladimir
[PATCH 2/6] block/mirror: fix use after free of local_err, Vladimir Sementsov-Ogievskiy, 2020/03/24