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

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 . )
+
+@ 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, ...)
+     ...>
+ }
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
 
 GDB stub
 M: Alex Bennée <alex.bennee@linaro.org>
-- 
2.21.0

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?

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

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

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

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

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.

> 
>> +
>> +@ 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 <alex.bennee@linaro.org>
> 


-- 
Best regards,
Vladimir

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

31.03.2020 16:14, Markus Armbruster wrote:
> 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?

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

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


-- 
Best regards,
Vladimir

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

> 31.03.2020 16:14, Markus Armbruster wrote:
>> 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?
>
> 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 <armbru@redhat.com>

On Tue, 31 Mar 2020 at 10:01, Markus Armbruster <armbru@redhat.com> wrote:
> 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.

I find it useful -- you (hopefully) get the actual command line
the original author used, rather than having to guess which
options might be significant. (eg the last one I added uses
--keep-comments --smpl-spacing --include-headers --dir target
which aren't all always used but which do all matter here for
getting the change that went in in the commit that used the script.)
Most of us use coccinelle as an occasional tool, not an
everyday one, so not having to look through the docs to figure
out the right rune is a benefit, even for the options that
we do use on pretty much every spatch run.

thanks
-- PMM

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 31 Mar 2020 at 10:01, Markus Armbruster <armbru@redhat.com> wrote:
>> 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.
>
> I find it useful -- you (hopefully) get the actual command line
> the original author used, rather than having to guess which
> options might be significant. (eg the last one I added uses
> --keep-comments --smpl-spacing --include-headers --dir target
> which aren't all always used but which do all matter here for
> getting the change that went in in the commit that used the script.)
> Most of us use coccinelle as an occasional tool, not an
> everyday one, so not having to look through the docs to figure
> out the right rune is a benefit, even for the options that
> we do use on pretty much every spatch run.

Generic instructions for using .cocci scripts should go into README.
Enough to get you started if you know nothing about Coccinelle.

Options that should always be used with a certain script should be
documented in that script.

Options that only affect work-flow, not the patch, I'd rather keep out
of the script.  If there are any we feel we should mention, do that in
README.  Example: --no-show-diff.

Brief instructions for how a patch was created should be included in the
commit message.  They should suffice for readers familiar with
Coccinelle.  Yes, there's a bit of redundancy with README and the
script.

On Wed, 1 Apr 2020 at 06:07, Markus Armbruster <armbru@redhat.com> wrote:

> Generic instructions for using .cocci scripts should go into README.
> Enough to get you started if you know nothing about Coccinelle.
>
> Options that should always be used with a certain script should be
> documented in that script.
>
> Options that only affect work-flow, not the patch, I'd rather keep out
> of the script.  If there are any we feel we should mention, do that in
> README.  Example: --no-show-diff.

But then as a coccinelle script author I need to know which of
the options I needed are standard, which are for-this-script-only,
and which are just 'workflow'. And as a reader I *still* need to
go and look through the README and look at this script and
then try to reconstitute what command line might have been
used. That's more work for the author *and* more work for the
reader than just "put the command line you used into the script
as a comment" -- so who's it benefiting?

thanks
-- PMM

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 1 Apr 2020 at 06:07, Markus Armbruster <armbru@redhat.com> wrote:
>
>> Generic instructions for using .cocci scripts should go into README.
>> Enough to get you started if you know nothing about Coccinelle.
>>
>> Options that should always be used with a certain script should be
>> documented in that script.
>>
>> Options that only affect work-flow, not the patch, I'd rather keep out
>> of the script.  If there are any we feel we should mention, do that in
>> README.  Example: --no-show-diff.
>
> But then as a coccinelle script author I need to know which of
> the options I needed are standard, which are for-this-script-only,
> and which are just 'workflow'.

If you're capable of writing a Coccinelle script that actually does what
you want, I bet you dollars to donuts that you can decide which options
actually affect the patch in comparably no time whatsoever ;)

If you prefer to bother your reader with your personal choices, that's
between you and your reviewers.  Myself, I prefer less noise around the
signal.

>                                And as a reader I *still* need to
> go and look through the README and look at this script and
> then try to reconstitute what command line might have been
> used.

You don't.  The "just work-flow" options by definition do not affect the
patch.

If you got Coccinelle installed and know the very basics, then the
incantation in the script should suffice to use the script, and the
incantation in the commit message should suffice to reproduce the patch.

If you know nothing about Coccinelle, the README should get you started.

Example:

    commit 4e20c1becba3fd2e8e71a2663cefb9627fd2a6e0
    Author: Markus Armbruster <armbru@redhat.com>
    Date:   Thu Dec 13 18:51:54 2018 +0100

        block: Replace qdict_put() by qdict_put_obj() where appropriate

        Patch created mechanically by rerunning:

          $  spatch --sp-file scripts/coccinelle/qobject.cocci \
                    --macro-file scripts/cocci-macro-file.h \
                    --dir block --in-place

        Signed-off-by: Markus Armbruster <armbru@redhat.com>
        Reviewed-by: Alberto Garcia <berto@igalia.com>
        Reviewed-by: Eric Blake <eblake@redhat.com>
        Signed-off-by: Kevin Wolf <kwolf@redhat.com>

scripts/coccinelle/qobject.cocci has no usage comment.  I doubt it needs
one, but I'd certainly tolerate something like

    // Usage:
    // spatch --sp-file scripts/coccinelle/qobject.cocci \
    //        --macro-file scripts/cocci-macro-file.h \
    //        FILES ...

I'd even tolerate throwing in --in-place.  But --use-gitgrep is too much
for my taste.

>       That's more work for the author *and* more work for the
> reader than just "put the command line you used into the script
> as a comment" -- so who's it benefiting?

Anyone with basic Coccinelle proficiency benefits slightly from the
reduction of noise.

Coccinelle noobs benefit from the more verbose instructions in the
README.  Duplicating those in every script is not maintainable.

Maintainers benefit slightly from less redundancy.

>> Brief instructions for how a patch was created should be included in the
>> commit message.  They should suffice for readers familiar with
>> Coccinelle.  Yes, there's a bit of redundancy with README and the
>> script.

On Wed, 1 Apr 2020 at 15:44, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> > On Wed, 1 Apr 2020 at 06:07, Markus Armbruster <armbru@redhat.com> wrote:
> > But then as a coccinelle script author I need to know which of
> > the options I needed are standard, which are for-this-script-only,
> > and which are just 'workflow'.
>
> If you're capable of writing a Coccinelle script that actually does what
> you want, I bet you dollars to donuts that you can decide which options
> actually affect the patch in comparably no time whatsoever ;)

I use this thing maybe once a month at most, more likely once
every three months, and the documentation is notoriously
impenetrable. I really really don't want to have to start looking in it
and guessing about how the original author ran the script, when
they could have just told me.

> If you prefer to bother your reader with your personal choices, that's
> between you and your reviewers.  Myself, I prefer less noise around the
> signal.

> If you got Coccinelle installed and know the very basics, then the
> incantation in the script should suffice to use the script, and the
> incantation in the commit message should suffice to reproduce the patch.

So I need to now look in the git log for the script to find the commit
message? Why not just put the command in the file and save steps?

> Example:
>
>     commit 4e20c1becba3fd2e8e71a2663cefb9627fd2a6e0
>     Author: Markus Armbruster <armbru@redhat.com>
>     Date:   Thu Dec 13 18:51:54 2018 +0100
>
>         block: Replace qdict_put() by qdict_put_obj() where appropriate
>
>         Patch created mechanically by rerunning:
>
>           $  spatch --sp-file scripts/coccinelle/qobject.cocci \
>                     --macro-file scripts/cocci-macro-file.h \
>                     --dir block --in-place

Yep, that command line would be great to see in the script file.

> scripts/coccinelle/qobject.cocci has no usage comment.  I doubt it needs
> one, but I'd certainly tolerate something like

    // Usage:
    // spatch --sp-file scripts/coccinelle/qobject.cocci \
    //        --macro-file scripts/cocci-macro-file.h \
    //        FILES ...

I think that should be about the minimum. I think every
.cocci file should say how it was used or is supposed to be used.
The least-effort way for the author of the script to do that is to
simply give the command line they used to run it.

> >       That's more work for the author *and* more work for the
> > reader than just "put the command line you used into the script
> > as a comment" -- so who's it benefiting?
>
> Anyone with basic Coccinelle proficiency benefits slightly from the
> reduction of noise.

How 'basic' is basic? I think that being specific is useful for
anybody who's at my level or lower (ie, can write a script, doesn't
do so often enough to be able to write a script or run spatch
without looking at documentation and cribbing from other scripts
as examples). How many people do we have at a higher level
than that for whom this is noise? 2? 3? And people who do
know Coccinelle well should have no difficulty in quickly
looking at a command line and mentally filtering out the options
that they don't feel they need.

thanks
-- PMM

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 1 Apr 2020 at 15:44, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>> > On Wed, 1 Apr 2020 at 06:07, Markus Armbruster <armbru@redhat.com> wrote:
>> > But then as a coccinelle script author I need to know which of
>> > the options I needed are standard, which are for-this-script-only,
>> > and which are just 'workflow'.
>>
>> If you're capable of writing a Coccinelle script that actually does what
>> you want, I bet you dollars to donuts that you can decide which options
>> actually affect the patch in comparably no time whatsoever ;)
>
> I use this thing maybe once a month at most, more likely once
> every three months, and the documentation is notoriously
> impenetrable. I really really don't want to have to start looking in it
> and guessing about how the original author ran the script, when
> they could have just told me.

I'm afraid we're talking part each other.

>> If you prefer to bother your reader with your personal choices, that's
>> between you and your reviewers.  Myself, I prefer less noise around the
>> signal.
>
>> If you got Coccinelle installed and know the very basics, then the
>> incantation in the script should suffice to use the script, and the
>> incantation in the commit message should suffice to reproduce the patch.
>
> So I need to now look in the git log for the script to find the commit
> message? Why not just put the command in the file and save steps?

I'm not opposed to usage comments in .cocci.

I *am* apposed to noise in usage comments.

>> Example:
>>
>>     commit 4e20c1becba3fd2e8e71a2663cefb9627fd2a6e0
>>     Author: Markus Armbruster <armbru@redhat.com>
>>     Date:   Thu Dec 13 18:51:54 2018 +0100
>>
>>         block: Replace qdict_put() by qdict_put_obj() where appropriate
>>
>>         Patch created mechanically by rerunning:
>>
>>           $  spatch --sp-file scripts/coccinelle/qobject.cocci \
>>                     --macro-file scripts/cocci-macro-file.h \
>>                     --dir block --in-place
>
> Yep, that command line would be great to see in the script file.

Except for the --dir block part, which is even worse than noise: it
suggests this is just for block/, which is wrong.

>> scripts/coccinelle/qobject.cocci has no usage comment.  I doubt it needs
>> one, but I'd certainly tolerate something like
>
>     // Usage:
>     // spatch --sp-file scripts/coccinelle/qobject.cocci \
>     //        --macro-file scripts/cocci-macro-file.h \
>     //        FILES ...
>
> I think that should be about the minimum. I think every
> .cocci file should say how it was used or is supposed to be used.

Fine with me.

> The least-effort way for the author of the script to do that is to
> simply give the command line they used to run it.

If you're capable of writing a Coccinelle script that actually does what
you want, you're certainly capable of doing better than blindly paste
from your shell history.  Kindly drop the options that are specific to
just this particular use of the script.  Keep the ones that future users
will actually need.

>> >       That's more work for the author *and* more work for the
>> > reader than just "put the command line you used into the script
>> > as a comment" -- so who's it benefiting?
>>
>> Anyone with basic Coccinelle proficiency benefits slightly from the
>> reduction of noise.
>
> How 'basic' is basic? I think that being specific is useful for
> anybody who's at my level or lower (ie, can write a script, doesn't
> do so often enough to be able to write a script or run spatch
> without looking at documentation and cribbing from other scripts
> as examples). How many people do we have at a higher level
> than that for whom this is noise? 2? 3? And people who do
> know Coccinelle well should have no difficulty in quickly
> looking at a command line and mentally filtering out the options
> that they don't feel they need.

Two proficiencies: using a script somebody else wrote, and writing
simple scripts.  Let me try to sketch just about the most basic of basic
levels for the former.  Note that I'm making *liberal* allowance for
reluctance to learn tools[*].

Understand

* that .cocci means Coccinelle
* how to install Coccinelle
* that you need to feed the .cocci to spatch
* that this produces a patch
* how to apply the patch to the tree

None of this I want to explain in every .cocci script.  All of this
I want be explained in scripts/coccinelle/README.


[*] Not a trait I like to see in craftsmen.

On Thu, 2 Apr 2020 at 07:55, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> > I use this thing maybe once a month at most, more likely once
> > every three months, and the documentation is notoriously
> > impenetrable. I really really don't want to have to start looking in it
> > and guessing about how the original author ran the script, when
> > they could have just told me.
>
> I'm afraid we're talking part each other.

Perhaps; but I think we're also genuinely disagreeing.

> >>           $  spatch --sp-file scripts/coccinelle/qobject.cocci \
> >>                     --macro-file scripts/cocci-macro-file.h \
> >>                     --dir block --in-place
> >
> > Yep, that command line would be great to see in the script file.
>
> Except for the --dir block part, which is even worse than noise: it
> suggests this is just for block/, which is wrong.

It tells the reader that the original author only tested
the script to work inside block/, which is useful information.
(This is why scripts/coccinelle/cpu-reset.cocci specifies
--dir target in its command.)

> > The least-effort way for the author of the script to do that is to
> > simply give the command line they used to run it.
>
> If you're capable of writing a Coccinelle script that actually does what
> you want, you're certainly capable of doing better than blindly paste
> from your shell history.  Kindly drop the options that are specific to
> just this particular use of the script.  Keep the ones that future users
> will actually need.

I'm a future user; in fact I'm the future user whose needs I have
the best information on. I want to see the whole command, please.

> Two proficiencies: using a script somebody else wrote, and writing
> simple scripts.  Let me try to sketch just about the most basic of basic
> levels for the former.  Note that I'm making *liberal* allowance for
> reluctance to learn tools[*].
>
> Understand
>
> * that .cocci means Coccinelle
> * how to install Coccinelle
> * that you need to feed the .cocci to spatch
> * that this produces a patch
> * how to apply the patch to the tree
>
> None of this I want to explain in every .cocci script.  All of this
> I want be explained in scripts/coccinelle/README.

I'm certainly not arguing against having a README which helps
people with onboarding into the world of coccinelle scripts;
I think that would be great. I just don't really see much benefit
to either authors or script readers in asking authors to do more
than quote the coccinelle command line they used in the
comments at the top of their script.

thanks
-- PMM

Peter Maydell <peter.maydell@linaro.org> writes:

> On Thu, 2 Apr 2020 at 07:55, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>> > I use this thing maybe once a month at most, more likely once
>> > every three months, and the documentation is notoriously
>> > impenetrable. I really really don't want to have to start looking in it
>> > and guessing about how the original author ran the script, when
>> > they could have just told me.
>>
>> I'm afraid we're talking part each other.
>
> Perhaps; but I think we're also genuinely disagreeing.
>
>> >>           $  spatch --sp-file scripts/coccinelle/qobject.cocci \
>> >>                     --macro-file scripts/cocci-macro-file.h \
>> >>                     --dir block --in-place
>> >
>> > Yep, that command line would be great to see in the script file.
>>
>> Except for the --dir block part, which is even worse than noise: it
>> suggests this is just for block/, which is wrong.
>
> It tells the reader that the original author only tested
> the script to work inside block/, which is useful information.
> (This is why scripts/coccinelle/cpu-reset.cocci specifies
> --dir target in its command.)
>
>> > The least-effort way for the author of the script to do that is to
>> > simply give the command line they used to run it.
>>
>> If you're capable of writing a Coccinelle script that actually does what
>> you want, you're certainly capable of doing better than blindly paste
>> from your shell history.  Kindly drop the options that are specific to
>> just this particular use of the script.  Keep the ones that future users
>> will actually need.
>
> I'm a future user; in fact I'm the future user whose needs I have
> the best information on. I want to see the whole command, please.

In that case, nothing seems to be left than agree to disagree.

[...]