[PATCH for-5.0 0/6] Several error use-after-free

[PATCH for-5.0 0/6] Several error use-after-free

[Vladimir Sementsov-Ogievskiy]

Hi all!

I accidentally found use-after-free of local_err in mirror, and decided
to search for similar cases with help of small coccinelle script
(patch 01). Happily, there no many cases.

Better to fix zero Error* pointer after each freeing everywhere, but
this is too much for 5.0 and most of these cases will be covered by
error-auto-propagation series.

Note also, that there are still a lot of use-after-free cases possible
when error is not local variable but field of some structure, shared by
several functions.

Vladimir Sementsov-Ogievskiy (6):
  scripts/coccinelle: add error-use-after-free.cocci
  block/mirror: fix use after free of local_err
  dump/win_dump: fix use after free of err
  migration/colo: fix use after free of local_err
  migration/ram: fix use after free of local_err
  qga/commands-posix: fix use after free of local_err

 scripts/coccinelle/error-use-after-free.cocci | 52 +++++++++++++++++++
 block/mirror.c                                |  1 +
 dump/win_dump.c                               |  4 +-
 migration/colo.c                              |  1 +
 migration/ram.c                               |  1 +
 qga/commands-posix.c                          |  3 ++
 MAINTAINERS                                   |  1 +
 7 files changed, 60 insertions(+), 3 deletions(-)
 create mode 100644 scripts/coccinelle/error-use-after-free.cocci

-- 
2.21.0

[PATCH 1/6] scripts/coccinelle: add error-use-after-free.cocci

[Vladimir Sementsov-Ogievskiy]

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

[PATCH 2/6] block/mirror: fix use after free of local_err

[Vladimir Sementsov-Ogievskiy]

local_err is used again in mirror_exit_common() after
bdrv_set_backing_hd(), so we must zero it. Otherwise try to set
non-NULL local_err will crash.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/mirror.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/mirror.c b/block/mirror.c
index 447051dbc6..6203e5946e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -678,6 +678,7 @@ static int mirror_exit_common(Job *job)
             bdrv_set_backing_hd(target_bs, backing, &local_err);
             if (local_err) {
                 error_report_err(local_err);
+                local_err = NULL;
                 ret = -EPERM;
             }
         }
-- 
2.21.0

[PATCH 3/6] dump/win_dump: fix use after free of err

[Vladimir Sementsov-Ogievskiy]

It's possible that we'll try to set err twice (or more). It's bad, it
will crash.

Instead, use warn_report().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 dump/win_dump.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/dump/win_dump.c b/dump/win_dump.c
index eda2a48974..652c7bad99 100644
--- a/dump/win_dump.c
+++ b/dump/win_dump.c
@@ -304,13 +304,11 @@ static void restore_context(WinDumpHeader64 *h,
                             struct saved_context *saved_ctx)
 {
     int i;
-    Error *err = NULL;
 
     for (i = 0; i < h->NumberProcessors; i++) {
         if (cpu_memory_rw_debug(first_cpu, saved_ctx[i].addr,
                 (uint8_t *)&saved_ctx[i].ctx, sizeof(WinContext), 1)) {
-            error_setg(&err, "win-dump: failed to restore CPU #%d context", i);
-            warn_report_err(err);
+            warn_report("win-dump: failed to restore CPU #%d context", i);
         }
     }
 }
-- 
2.21.0

[PATCH 4/6] migration/colo: fix use after free of local_err

[Vladimir Sementsov-Ogievskiy]

local_err is used again in secondary_vm_do_failover() after
replication_stop_all(), so we must zero it. Otherwise try to set
non-NULL local_err will crash.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 migration/colo.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/colo.c b/migration/colo.c
index 44942c4e23..a54ac84f41 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -93,6 +93,7 @@ static void secondary_vm_do_failover(void)
     replication_stop_all(true, &local_err);
     if (local_err) {
         error_report_err(local_err);
+        local_err = NULL;
     }
 
     /* Notify all filters of all NIC to do checkpoint */
-- 
2.21.0

[PATCH 5/6] migration/ram: fix use after free of local_err

[Vladimir Sementsov-Ogievskiy]

local_err is used again in migration_bitmap_sync_precopy() after
precopy_notify(), so we must zero it. Otherwise try to set
non-NULL local_err will crash.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 migration/ram.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/ram.c b/migration/ram.c
index c12cfdbe26..04f13feb2e 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -980,6 +980,7 @@ static void migration_bitmap_sync_precopy(RAMState *rs)
      */
     if (precopy_notify(PRECOPY_NOTIFY_BEFORE_BITMAP_SYNC, &local_err)) {
         error_report_err(local_err);
+        local_err = NULL;
     }
 
     migration_bitmap_sync(rs);
-- 
2.21.0

[PATCH 6/6] qga/commands-posix: fix use after free of local_err

[Vladimir Sementsov-Ogievskiy]

local_err is used several times in guest_suspend(). Setting non-NULL
local_err will crash, so let's zero it after freeing. Also fix possible
leak of local_err in final if().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qga/commands-posix.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 93474ff770..cc69b82704 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1773,6 +1773,7 @@ static void guest_suspend(SuspendMode mode, Error **errp)
     }
 
     error_free(local_err);
+    local_err = NULL;
 
     if (pmutils_supports_mode(mode, &local_err)) {
         mode_supported = true;
@@ -1784,6 +1785,7 @@ static void guest_suspend(SuspendMode mode, Error **errp)
     }
 
     error_free(local_err);
+    local_err = NULL;
 
     if (linux_sys_state_supports_mode(mode, &local_err)) {
         mode_supported = true;
@@ -1791,6 +1793,7 @@ static void guest_suspend(SuspendMode mode, Error **errp)
     }
 
     if (!mode_supported) {
+        error_free(local_err);
         error_setg(errp,
                    "the requested suspend mode is not supported by the guest");
     } else {
-- 
2.21.0

Re: [PATCH for-5.0 0/6] Several error use-after-free

[Richard Henderson]

On 3/24/20 8:36 AM, Vladimir Sementsov-Ogievskiy wrote:
> Vladimir Sementsov-Ogievskiy (6):
>   scripts/coccinelle: add error-use-after-free.cocci
>   block/mirror: fix use after free of local_err
>   dump/win_dump: fix use after free of err
>   migration/colo: fix use after free of local_err
>   migration/ram: fix use after free of local_err
>   qga/commands-posix: fix use after free of local_err

Whole series:
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 2/6] block/mirror: fix use after free of local_err

[Eric Blake]

On 3/24/20 10:36 AM, Vladimir Sementsov-Ogievskiy wrote:
> local_err is used again in mirror_exit_common() after
> bdrv_set_backing_hd(), so we must zero it. Otherwise try to set
> non-NULL local_err will crash.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/mirror.c | 1 +
>   1 file changed, 1 insertion(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 447051dbc6..6203e5946e 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -678,6 +678,7 @@ static int mirror_exit_common(Job *job)
>               bdrv_set_backing_hd(target_bs, backing, &local_err);
>               if (local_err) {
>                   error_report_err(local_err);
> +                local_err = NULL;
>                   ret = -EPERM;
>               }
>           }
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 2/6] block/mirror: fix use after free of local_err

[John Snow]

On 3/24/20 11:36 AM, Vladimir Sementsov-Ogievskiy wrote:
> local_err is used again in mirror_exit_common() after
> bdrv_set_backing_hd(), so we must zero it. Otherwise try to set
> non-NULL local_err will crash.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/mirror.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 447051dbc6..6203e5946e 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -678,6 +678,7 @@ static int mirror_exit_common(Job *job)
>              bdrv_set_backing_hd(target_bs, backing, &local_err);
>              if (local_err) {
>                  error_report_err(local_err);
> +                local_err = NULL;
>                  ret = -EPERM;
>              }
>          }
> 

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [PATCH 4/6] migration/colo: fix use after free of local_err

[Dr. David Alan Gilbert]

* Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
> local_err is used again in secondary_vm_do_failover() after
> replication_stop_all(), so we must zero it. Otherwise try to set
> non-NULL local_err will crash.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

I'll queue this

> ---
>  migration/colo.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/migration/colo.c b/migration/colo.c
> index 44942c4e23..a54ac84f41 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -93,6 +93,7 @@ static void secondary_vm_do_failover(void)
>      replication_stop_all(true, &local_err);
>      if (local_err) {
>          error_report_err(local_err);
> +        local_err = NULL;
>      }
>  
>      /* Notify all filters of all NIC to do checkpoint */
> -- 
> 2.21.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 5/6] migration/ram: fix use after free of local_err

[Dr. David Alan Gilbert]

* Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
> local_err is used again in migration_bitmap_sync_precopy() after
> precopy_notify(), so we must zero it. Otherwise try to set
> non-NULL local_err will crash.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  migration/ram.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index c12cfdbe26..04f13feb2e 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -980,6 +980,7 @@ static void migration_bitmap_sync_precopy(RAMState *rs)
>       */
>      if (precopy_notify(PRECOPY_NOTIFY_BEFORE_BITMAP_SYNC, &local_err)) {
>          error_report_err(local_err);
> +        local_err = NULL;

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

and queued.


>      }
>  
>      migration_bitmap_sync(rs);
> -- 
> 2.21.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 6/6] qga/commands-posix: fix use after free of local_err

[Eric Blake]

On 3/24/20 10:36 AM, Vladimir Sementsov-Ogievskiy wrote:
> local_err is used several times in guest_suspend(). Setting non-NULL
> local_err will crash, so let's zero it after freeing. Also fix possible
> leak of local_err in final if().
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   qga/commands-posix.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 93474ff770..cc69b82704 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1773,6 +1773,7 @@ static void guest_suspend(SuspendMode mode, Error **errp)
>       }
>   
>       error_free(local_err);
> +    local_err = NULL;

Let's show this with more context.

> static void guest_suspend(SuspendMode mode, Error **errp)
> {
>     Error *local_err = NULL;
>     bool mode_supported = false;
> 
>     if (systemd_supports_mode(mode, &local_err)) {

Hmm - we have an even earlier bug that needs fixing.  Note that 
systemd_supports_mode() returns a bool AND conditionally sets errp.  But 
it is inconsistent: it has the following table of actions based on the 
results of run_process_child() on "systemctl status" coupled with the 
man page on "systemctl status" return values:
-1 (unable to run systemctl) -> errp set, return false
0 (unit is active) -> errp left unchanged, return false
1 (unit not failed) -> errp left unchanged, return true
2 (unused) -> errp left unchanged, return true
3 (unit not active) -> errp left unchanged, return true
4 (no such unit) -> errp left unchanged, return false
5+ (unexpected from systemctl) -> errp left unchanged, return false

But the comments in systemd_supports_mode() claim that ANY status < 4 
(other than -1, which means we did not run systemctl) should count as 
the service existing, even though the most common status is 3.  If our 
comment is to be believed, then we should return true, not false, for 
status 0.

Now, back to _this_ function:

>         mode_supported = true;
>         systemd_suspend(mode, &local_err);

Okay - if we get here (whether from status 1-3, or with 
systemd_supports_mode fixed to support status 0-3), local_err is still 
unset prior to calling systemd_suspend(), and we are guaranteed that 
after the call, either we suspended successfully or local_err is now set.

>     }
> 
>     if (!local_err) {
>         return;
>     }

So if returned, we succeeded at systemd_suspend, and there is nothing 
further to do; but if we get past that point, we don't know if it was 
systemd_supports_mode that failed or systemd_suspend that failed, and we 
don't know if local_err is set.

> 
>     error_free(local_err);
> +    local_err = NULL;

Yet, we blindly throw away local_err, without trying to report it.  If 
that's the case, then WHY are we passing in local_err?  Wouldn't it be 
better to pass in NULL (we really don't care about the error message), 
and/or fix systemd_suspend() to return a bool just like 
systemd_supports_mode, and/or fix systemd_supports_mode to guarantee 
that it sets errp when returning false?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 6/6] qga/commands-posix: fix use after free of local_err

[Vladimir Sementsov-Ogievskiy]

24.03.2020 23:03, Eric Blake wrote:
> On 3/24/20 10:36 AM, Vladimir Sementsov-Ogievskiy wrote:
>> local_err is used several times in guest_suspend(). Setting non-NULL
>> local_err will crash, so let's zero it after freeing. Also fix possible
>> leak of local_err in final if().
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qga/commands-posix.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 93474ff770..cc69b82704 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -1773,6 +1773,7 @@ static void guest_suspend(SuspendMode mode, Error **errp)
>>       }
>>       error_free(local_err);
>> +    local_err = NULL;
> 
> Let's show this with more context.
> 
>> static void guest_suspend(SuspendMode mode, Error **errp)
>> {
>>     Error *local_err = NULL;
>>     bool mode_supported = false;
>>
>>     if (systemd_supports_mode(mode, &local_err)) {
> 
> Hmm - we have an even earlier bug that needs fixing.  Note that systemd_supports_mode() returns a bool AND conditionally sets errp.  But it is inconsistent: it has the following table of actions based on the results of run_process_child() on "systemctl status" coupled with the man page on "systemctl status" return values:
> -1 (unable to run systemctl) -> errp set, return false
> 0 (unit is active) -> errp left unchanged, return false
> 1 (unit not failed) -> errp left unchanged, return true
> 2 (unused) -> errp left unchanged, return true
> 3 (unit not active) -> errp left unchanged, return true
> 4 (no such unit) -> errp left unchanged, return false
> 5+ (unexpected from systemctl) -> errp left unchanged, return false
> 
> But the comments in systemd_supports_mode() claim that ANY status < 4 (other than -1, which means we did not run systemctl) should count as the service existing, even though the most common status is 3.  If our comment is to be believed, then we should return true, not false, for status 0.
> 
> Now, back to _this_ function:
> 
>>         mode_supported = true;
>>         systemd_suspend(mode, &local_err);
> 
> Okay - if we get here (whether from status 1-3, or with systemd_supports_mode fixed to support status 0-3), local_err is still unset prior to calling systemd_suspend(), and we are guaranteed that after the call, either we suspended successfully or local_err is now set.
> 
>>     }
>>
>>     if (!local_err) {
>>         return;
>>     }
> 
> So if returned, we succeeded at systemd_suspend, and there is nothing further to do; but if we get past that point, we don't know if it was systemd_supports_mode that failed or systemd_suspend that failed, and we don't know if local_err is set.

No, we know that is set, as we check exactly this and return if not set.

> 
>>
>>     error_free(local_err);
>> +    local_err = NULL;
> 
> Yet, we blindly throw away local_err, without trying to report it.  If that's the case, then WHY are we passing in local_err?  Wouldn't it be better to pass in NULL (we really don't care about the error message), and/or fix systemd_suspend() to return a bool just like systemd_supports_mode, and/or fix systemd_supports_mode to guarantee that it sets errp when returning false?
> 

I agree that this is a strange function and its logic is weird. But I don't know what the logic should be. My patch is still valid to just fix obvious use-after-free and possible leak. It doesn't fix the logic.


-- 
Best regards,
Vladimir

Re: [PATCH 2/6] block/mirror: fix use after free of local_err

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1218 bytes --]

On 24.03.20 16:36, Vladimir Sementsov-Ogievskiy wrote:
> local_err is used again in mirror_exit_common() after
> bdrv_set_backing_hd(), so we must zero it. Otherwise try to set
> non-NULL local_err will crash.

OK, but wouldn’t it be better hygiene to set it to NULL every time it is
freed?  (There is a second instance of error_report_err() in this
function.  I’m a bit worried we might introduce another local_err use
after that one at some point in the future, and forget to run the cocci
script then.)

Are the cocci scripts run regularly by someone?  E.g. as part of a pull
to master?

Max

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/mirror.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 447051dbc6..6203e5946e 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -678,6 +678,7 @@ static int mirror_exit_common(Job *job)
>              bdrv_set_backing_hd(target_bs, backing, &local_err);
>              if (local_err) {
>                  error_report_err(local_err);
> +                local_err = NULL;
>                  ret = -EPERM;
>              }
>          }
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/6] block/mirror: fix use after free of local_err

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 770 bytes --]

On 25.03.20 12:11, Max Reitz wrote:
> On 24.03.20 16:36, Vladimir Sementsov-Ogievskiy wrote:
>> local_err is used again in mirror_exit_common() after
>> bdrv_set_backing_hd(), so we must zero it. Otherwise try to set
>> non-NULL local_err will crash.
> 
> OK, but wouldn’t it be better hygiene to set it to NULL every time it is
> freed?  (There is a second instance of error_report_err() in this
> function.  I’m a bit worried we might introduce another local_err use
> after that one at some point in the future, and forget to run the cocci
> script then.)
> 
> Are the cocci scripts run regularly by someone?  E.g. as part of a pull
> to master?

Doesn’t look like it.  I’m currently running everything, and there’s a
lot of results so far.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/6] block/mirror: fix use after free of local_err

[Vladimir Sementsov-Ogievskiy]

25.03.2020 14:11, Max Reitz wrote:
> On 24.03.20 16:36, Vladimir Sementsov-Ogievskiy wrote:
>> local_err is used again in mirror_exit_common() after
>> bdrv_set_backing_hd(), so we must zero it. Otherwise try to set
>> non-NULL local_err will crash.
> 
> OK, but wouldn’t it be better hygiene to set it to NULL every time it is
> freed?  (There is a second instance of error_report_err() in this
> function.  I’m a bit worried we might introduce another local_err use
> after that one at some point in the future, and forget to run the cocci
> script then.)

Yes, it's better. But if we now decide to fix all such things, it would be
huge series. May be too huge for 5.0..

So I decided to fix only real obvious problems now.

Hmm huge or not?

Ok, let's try with less restrictions:

--- a/scripts/coccinelle/error-use-after-free.cocci
+++ b/scripts/coccinelle/error-use-after-free.cocci
@@ -28,7 +28,7 @@ expression err;

   fn(...)
   {
-     <...
+     ... when any
  (
       error_free(err);
  +    err = NULL;
@@ -46,7 +46,5 @@ expression err;
  +    err = NULL;
  )
       ... when != err = NULL
-         when != exit(...)
-     fn2(..., err, ...)
-     ...>
+         when any
   }


on block/ directory:

spatch --sp-file scripts/coccinelle/error-use-after-free.cocci --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff --use-gitgrep block
git diff --stat
  scripts/coccinelle/error-use-after-free.cocci |  6 ++----
  block/block-backend.c                         |  1 +
  block/commit.c                                |  4 ++++
  block/crypto.c                                |  1 +
  block/file-posix.c                            |  5 +++++
  block/mirror.c                                |  2 ++
  block/monitor/block-hmp-cmds.c                |  4 ++++
  block/parallels.c                             |  3 +++
  block/qapi-sysemu.c                           |  2 ++
  block/qapi.c                                  |  1 +
  block/qcow.c                                  |  2 ++
  block/qcow2-cluster.c                         |  1 +
  block/qcow2-refcount.c                        |  1 +
  block/qcow2-snapshot.c                        |  3 +++
  block/qcow2.c                                 |  4 ++++
  block/replication.c                           |  1 +
  block/sheepdog.c                              | 13 +++++++++++++
  block/stream.c                                |  1 +
  block/vdi.c                                   |  2 ++
  block/vhdx.c                                  |  2 ++
  block/vmdk.c                                  |  2 ++
  block/vpc.c                                   |  2 ++
  block/vvfat.c                                 |  2 ++
  23 files changed, 61 insertions(+), 4 deletions(-)


If you want, I'll send series.

> 
> Are the cocci scripts run regularly by someone?  E.g. as part of a pull
> to master?

I'm afraid not. I have a plan in my mind, to make python checkcode, which will
work in pair with checkpatch somehow, and will work on workdir instead of
patch. It will simplify significantly adding different code checks, including
starting coccinelle scripts.

> 
> Max
> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/mirror.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index 447051dbc6..6203e5946e 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -678,6 +678,7 @@ static int mirror_exit_common(Job *job)
>>               bdrv_set_backing_hd(target_bs, backing, &local_err);
>>               if (local_err) {
>>                   error_report_err(local_err);
>> +                local_err = NULL;
>>                   ret = -EPERM;
>>               }
>>           }
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [PATCH 2/6] block/mirror: fix use after free of local_err

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 4361 bytes --]

On 25.03.20 12:47, Vladimir Sementsov-Ogievskiy wrote:
> 25.03.2020 14:11, Max Reitz wrote:
>> On 24.03.20 16:36, Vladimir Sementsov-Ogievskiy wrote:
>>> local_err is used again in mirror_exit_common() after
>>> bdrv_set_backing_hd(), so we must zero it. Otherwise try to set
>>> non-NULL local_err will crash.
>>
>> OK, but wouldn’t it be better hygiene to set it to NULL every time it is
>> freed?  (There is a second instance of error_report_err() in this
>> function.  I’m a bit worried we might introduce another local_err use
>> after that one at some point in the future, and forget to run the cocci
>> script then.)
> 
> Yes, it's better. But if we now decide to fix all such things, it would be
> huge series. May be too huge for 5.0..
> 
> So I decided to fix only real obvious problems now.

Reasonable, yes.

> Hmm huge or not?
> 
> Ok, let's try with less restrictions:
> 
> --- a/scripts/coccinelle/error-use-after-free.cocci
> +++ b/scripts/coccinelle/error-use-after-free.cocci
> @@ -28,7 +28,7 @@ expression err;
> 
>   fn(...)
>   {
> -     <...
> +     ... when any
>  (
>       error_free(err);
>  +    err = NULL;
> @@ -46,7 +46,5 @@ expression err;
>  +    err = NULL;
>  )
>       ... when != err = NULL
> -         when != exit(...)
> -     fn2(..., err, ...)
> -     ...>
> +         when any
>   }
> 
> 
> on block/ directory:
> 
> spatch --sp-file scripts/coccinelle/error-use-after-free.cocci
> --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff
> --use-gitgrep block
> git diff --stat
>  scripts/coccinelle/error-use-after-free.cocci |  6 ++----
>  block/block-backend.c                         |  1 +
>  block/commit.c                                |  4 ++++
>  block/crypto.c                                |  1 +
>  block/file-posix.c                            |  5 +++++
>  block/mirror.c                                |  2 ++
>  block/monitor/block-hmp-cmds.c                |  4 ++++
>  block/parallels.c                             |  3 +++
>  block/qapi-sysemu.c                           |  2 ++
>  block/qapi.c                                  |  1 +
>  block/qcow.c                                  |  2 ++
>  block/qcow2-cluster.c                         |  1 +
>  block/qcow2-refcount.c                        |  1 +
>  block/qcow2-snapshot.c                        |  3 +++
>  block/qcow2.c                                 |  4 ++++
>  block/replication.c                           |  1 +
>  block/sheepdog.c                              | 13 +++++++++++++
>  block/stream.c                                |  1 +
>  block/vdi.c                                   |  2 ++
>  block/vhdx.c                                  |  2 ++
>  block/vmdk.c                                  |  2 ++
>  block/vpc.c                                   |  2 ++
>  block/vvfat.c                                 |  2 ++
>  23 files changed, 61 insertions(+), 4 deletions(-)
> 
> 
> If you want, I'll send series.
> 
>>
>> Are the cocci scripts run regularly by someone?  E.g. as part of a pull
>> to master?
> 
> I'm afraid not. I have a plan in my mind, to make python checkcode,
> which will
> work in pair with checkpatch somehow, and will work on workdir instead of
> patch. It will simplify significantly adding different code checks,
> including
> starting coccinelle scripts.
Hm.  I think we need to prepare for noone running the cocci scripts
(i.e., we should use the above variant with less restrictions so that
future patches are less likely to reintroduce the problem), or we need
to ensure the cocci scripts are run regularly.

We can of course also do both.  In this case I think it makes sense to
do a less-restricted version, because I think it can never hurt to set
pointers to NULL after freeing them.  (We could do an exception for when
the error-freeing is immediately followed by a goto out, but I think
that would make it too complicated.)

I’d like to start running the cocci scripts myself before every pull
request, but unfortunately there are still a number of diffs in the
block area.  I think I’ll send a series to fix those and then I can run
the scripts regularly to prevent regressions.  So I’ll leave it up to
you whether you think a less-restricted version would make sense.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/6] block/mirror: fix use after free of local_err

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 600 bytes --]

On 24.03.20 16:36, Vladimir Sementsov-Ogievskiy wrote:
> local_err is used again in mirror_exit_common() after
> bdrv_set_backing_hd(), so we must zero it. Otherwise try to set
> non-NULL local_err will crash.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/mirror.c | 1 +
>  1 file changed, 1 insertion(+)

Considering Dave has taken patches 4 and 5, I think it makes sense for
me to take this one now; so, thanks for the patch and the reviews,
applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/6] block/mirror: fix use after free of local_err

[Eric Blake]

On 3/25/20 6:11 AM, Max Reitz wrote:
> On 24.03.20 16:36, Vladimir Sementsov-Ogievskiy wrote:
>> local_err is used again in mirror_exit_common() after
>> bdrv_set_backing_hd(), so we must zero it. Otherwise try to set
>> non-NULL local_err will crash.
> 
> OK, but wouldn’t it be better hygiene to set it to NULL every time it is
> freed?

If we change the signature to error_report_err(&local_err), where 
error_report_err both reports the error (if any) AND sets local_err to 
NULL, then we fix the problem for all callers.  It's a global 
search-and-replace job (Coccinelle is great for that) to update all 
callers to the new signature.

>  (There is a second instance of error_report_err() in this
> function.  I’m a bit worried we might introduce another local_err use
> after that one at some point in the future, and forget to run the cocci
> script then.)
> 
> Are the cocci scripts run regularly by someone?  E.g. as part of a pull
> to master?

I'm not aware of any automated procedure for it at the moment; rather, 
it is still ad hoc as someone notices something needs to be re-run.  But 
there was another thread about someone considering automating Cocci 
scripts as part of the Euler robot...

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 3/6] dump/win_dump: fix use after free of err

[Markus Armbruster]

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

> It's possible that we'll try to set err twice (or more). It's bad, it
> will crash.

True.

> Instead, use warn_report().

Improvement even without the potential crash enabled by the loop.

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  dump/win_dump.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/dump/win_dump.c b/dump/win_dump.c
> index eda2a48974..652c7bad99 100644
> --- a/dump/win_dump.c
> +++ b/dump/win_dump.c
> @@ -304,13 +304,11 @@ static void restore_context(WinDumpHeader64 *h,
>                              struct saved_context *saved_ctx)
>  {
>      int i;
> -    Error *err = NULL;
>  
>      for (i = 0; i < h->NumberProcessors; i++) {
>          if (cpu_memory_rw_debug(first_cpu, saved_ctx[i].addr,
>                  (uint8_t *)&saved_ctx[i].ctx, sizeof(WinContext), 1)) {
> -            error_setg(&err, "win-dump: failed to restore CPU #%d context", i);
> -            warn_report_err(err);
> +            warn_report("win-dump: failed to restore CPU #%d context", i);
>          }
>      }
>  }

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 6/6] qga/commands-posix: fix use after free of local_err

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 3/24/20 10:36 AM, Vladimir Sementsov-Ogievskiy wrote:
>> local_err is used several times in guest_suspend(). Setting non-NULL
>> local_err will crash, so let's zero it after freeing. Also fix possible
>> leak of local_err in final if().
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qga/commands-posix.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 93474ff770..cc69b82704 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -1773,6 +1773,7 @@ static void guest_suspend(SuspendMode mode, Error **errp)
>>       }
>>         error_free(local_err);
>> +    local_err = NULL;
>
> Let's show this with more context.
>
>> static void guest_suspend(SuspendMode mode, Error **errp)
>> {
>>     Error *local_err = NULL;
>>     bool mode_supported = false;
>>
>>     if (systemd_supports_mode(mode, &local_err)) {
>
> Hmm - we have an even earlier bug that needs fixing.  Note that
> systemd_supports_mode() returns a bool AND conditionally sets errp.
> But it is inconsistent: it has the following table of actions based on
> the results of run_process_child() on "systemctl status" coupled with
> the man page on "systemctl status" return values:
> -1 (unable to run systemctl) -> errp set, return false
> 0 (unit is active) -> errp left unchanged, return false
> 1 (unit not failed) -> errp left unchanged, return true
> 2 (unused) -> errp left unchanged, return true
> 3 (unit not active) -> errp left unchanged, return true
> 4 (no such unit) -> errp left unchanged, return false
> 5+ (unexpected from systemctl) -> errp left unchanged, return false

Three coarser cases:

* systemd_supports_mode() returned false with @local_err set
* systemd_supports_mode() returned false with @local_err clear
* systemd_supports_mode() returned true with @local_err clear

GLib specificially advises against the second case with GError:

    By convention, if you return a boolean value indicating success then
    TRUE means success and FALSE means failure.  Avoid creating
    functions which have a boolean return value and a GError parameter,
    but where the boolean does something other than signal whether the
    GError is set.

    https://developer.gnome.org/glib/stable/glib-Error-Reporting.html

In my opinion, the advice applies to our Error just as much.

> But the comments in systemd_supports_mode() claim that ANY status < 4
> (other than -1, which means we did not run systemctl) should count as
> the service existing, even though the most common status is 3.  If our
> comment is to be believed, then we should return true, not false, for
> status 0.
>
> Now, back to _this_ function:
>
>>         mode_supported = true;
>>         systemd_suspend(mode, &local_err);
>
> Okay - if we get here (whether from status 1-3, or with
> systemd_supports_mode fixed to support status 0-3), local_err is still
> unset prior to calling systemd_suspend(), and we are guaranteed that
> after the call, either we suspended successfully or local_err is now
> set.
>
>>     }

The conditional code splits the third case.  Result:

* systemd_supports_mode() returned false with @local_err set
* systemd_supports_mode() returned false with @local_err clear
* systemd_supports_mode() returned true, systemd_suspend() failed,
  @local_err set
* systemd_supports_mode() returned true, systemd_suspend() succeeded,
  @local_err clear

>>
>>     if (!local_err) {
>>         return;
>>     }
>
> So if returned, we succeeded at systemd_suspend, and there is nothing
> further to do; but if we get past that point, we don't know if it was
> systemd_supports_mode that failed or systemd_suspend that failed, and
> we don't know if local_err is set.
>
>>
>>     error_free(local_err);
>> +    local_err = NULL;

We use @local_err as one bit of information.

> Yet, we blindly throw away local_err, without trying to report it.  If
> that's the case, then WHY are we passing in local_err?  Wouldn't it be
> better to pass in NULL (we really don't care about the error message),
> and/or fix systemd_suspend() to return a bool just like
> systemd_supports_mode, and/or fix systemd_supports_mode to guarantee
> that it sets errp when returning false?

You're right, these interfaces are awkward.  They're used just here, so
there's no excuse for keeping them awkward.

Let's step back and examine what we're trying to do.  Pseudo-code:

    for method in systemd, pmutils, linux_sys_state:
        if method supports mode:
            try method
            if successful:
                return success

    if no method supports mode:
        return failure "the requested suspend mode is not supported by the guest"
    // we tried at least one method
    return the last method's failure

Observations:

1. We can abstract from the methods, or we can unroll the loop.
   Unrolling seems simpler here.

2. 'Method supports mode' is used as a simple predicate.  So make it
   one: return bool, and not take an Error ** argument.

3. The error for 'try method' is ignored except for the last try.  I'm
   not sure reporting just the last one is appropriate, but let's assume
   it is.  Preferred solution: make 'try method' return true on success,
   false on failure, ignore error (by passing null) unless we actually
   need it.  Acceptable solution: keep it void, free the Error objects
   we ignore.

Re: [PATCH 1/6] scripts/coccinelle: add error-use-after-free.cocci

[Markus Armbruster]

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>

Re: [PATCH 2/6] block/mirror: fix use after free of local_err

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 25.03.20 12:47, Vladimir Sementsov-Ogievskiy wrote:
>> 25.03.2020 14:11, Max Reitz wrote:
>>> On 24.03.20 16:36, Vladimir Sementsov-Ogievskiy wrote:
>>>> local_err is used again in mirror_exit_common() after
>>>> bdrv_set_backing_hd(), so we must zero it. Otherwise try to set
>>>> non-NULL local_err will crash.
>>>
>>> OK, but wouldn’t it be better hygiene to set it to NULL every time it is
>>> freed?  (There is a second instance of error_report_err() in this
>>> function.  I’m a bit worried we might introduce another local_err use
>>> after that one at some point in the future, and forget to run the cocci
>>> script then.)
>> 
>> Yes, it's better. But if we now decide to fix all such things, it would be
>> huge series. May be too huge for 5.0..
>> 
>> So I decided to fix only real obvious problems now.
>
> Reasonable, yes.

In particular since we have a tree-wide transformation waiting for 5.1.

>> Hmm huge or not?
>> 
>> Ok, let's try with less restrictions:
>> 
>> --- a/scripts/coccinelle/error-use-after-free.cocci
>> +++ b/scripts/coccinelle/error-use-after-free.cocci
>> @@ -28,7 +28,7 @@ expression err;
>> 
>>   fn(...)
>>   {
>> -     <...
>> +     ... when any
>>  (
>>       error_free(err);
>>  +    err = NULL;
>> @@ -46,7 +46,5 @@ expression err;
>>  +    err = NULL;
>>  )
>>       ... when != err = NULL
>> -         when != exit(...)
>> -     fn2(..., err, ...)
>> -     ...>
>> +         when any
>>   }
>> 
>> 
>> on block/ directory:
>> 
>> spatch --sp-file scripts/coccinelle/error-use-after-free.cocci
>> --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff
>> --use-gitgrep block
>> git diff --stat
>>  scripts/coccinelle/error-use-after-free.cocci |  6 ++----
>>  block/block-backend.c                         |  1 +
>>  block/commit.c                                |  4 ++++
>>  block/crypto.c                                |  1 +
>>  block/file-posix.c                            |  5 +++++
>>  block/mirror.c                                |  2 ++
>>  block/monitor/block-hmp-cmds.c                |  4 ++++
>>  block/parallels.c                             |  3 +++
>>  block/qapi-sysemu.c                           |  2 ++
>>  block/qapi.c                                  |  1 +
>>  block/qcow.c                                  |  2 ++
>>  block/qcow2-cluster.c                         |  1 +
>>  block/qcow2-refcount.c                        |  1 +
>>  block/qcow2-snapshot.c                        |  3 +++
>>  block/qcow2.c                                 |  4 ++++
>>  block/replication.c                           |  1 +
>>  block/sheepdog.c                              | 13 +++++++++++++
>>  block/stream.c                                |  1 +
>>  block/vdi.c                                   |  2 ++
>>  block/vhdx.c                                  |  2 ++
>>  block/vmdk.c                                  |  2 ++
>>  block/vpc.c                                   |  2 ++
>>  block/vvfat.c                                 |  2 ++
>>  23 files changed, 61 insertions(+), 4 deletions(-)
>> 
>> 
>> If you want, I'll send series.
>> 
>>>
>>> Are the cocci scripts run regularly by someone?  E.g. as part of a pull
>>> to master?
>> 
>> I'm afraid not. I have a plan in my mind, to make python checkcode,
>> which will
>> work in pair with checkpatch somehow, and will work on workdir instead of
>> patch. It will simplify significantly adding different code checks,
>> including
>> starting coccinelle scripts.

CI also runs make check, so that's another place you can hook into.

Not sure Coccinelle is fast enough for running it all the time.

> Hm.  I think we need to prepare for noone running the cocci scripts
> (i.e., we should use the above variant with less restrictions so that
> future patches are less likely to reintroduce the problem), or we need
> to ensure the cocci scripts are run regularly.
>
> We can of course also do both.  In this case I think it makes sense to
> do a less-restricted version, because I think it can never hurt to set
> pointers to NULL after freeing them.  (We could do an exception for when
> the error-freeing is immediately followed by a goto out, but I think
> that would make it too complicated.)

Same reasoning applies to all kinds of resource deallocation, not just
error_free().  Perhaps the world should use g_free() & friends only via
g_clear_pointer().  For better or worse, it doesn't.

Related: "[PATCH v7 01/11] qapi/error: add (Error **errp) cleaning
APIs".

> I’d like to start running the cocci scripts myself before every pull
> request, but unfortunately there are still a number of diffs in the
> block area.  I think I’ll send a series to fix those and then I can run
> the scripts regularly to prevent regressions.  So I’ll leave it up to
> you whether you think a less-restricted version would make sense.
>
> Max

Re: [PATCH 1/6] scripts/coccinelle: add error-use-after-free.cocci

[Vladimir Sementsov-Ogievskiy]

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

Re: [PATCH 6/6] qga/commands-posix: fix use after free of local_err

[Markus Armbruster]

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

[...]
> I agree that this is a strange function and its logic is weird. But I
> don't know what the logic should be. My patch is still valid to just
> fix obvious use-after-free and possible leak. It doesn't fix the
> logic.

I sketched improved logic elsewhere in this thread, and I can turn that
into a patch.

I can either make it replace Vladimir's patch, or make it go on top.  If
the latter, we can apply just Vladimir's patch for 5.0, and punt mine to
5.1

Got a preference?

Re: [PATCH 6/6] qga/commands-posix: fix use after free of local_err

[Vladimir Sementsov-Ogievskiy]

31.03.2020 14:46, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
> [...]
>> I agree that this is a strange function and its logic is weird. But I
>> don't know what the logic should be. My patch is still valid to just
>> fix obvious use-after-free and possible leak. It doesn't fix the
>> logic.
> 
> I sketched improved logic elsewhere in this thread, and I can turn that
> into a patch.
> 
> I can either make it replace Vladimir's patch, or make it go on top.  If
> the latter, we can apply just Vladimir's patch for 5.0, and punt mine to
> 5.1
> 
> Got a preference?
> 

I don't.

-- 
Best regards,
Vladimir

Re: [PATCH 1/6] scripts/coccinelle: add error-use-after-free.cocci

[Markus Armbruster]

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

Re: [PATCH 1/6] scripts/coccinelle: add error-use-after-free.cocci

[Vladimir Sementsov-Ogievskiy]

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

Re: [PATCH 1/6] scripts/coccinelle: add error-use-after-free.cocci

[Markus Armbruster]

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>

Re: [PATCH 1/6] scripts/coccinelle: add error-use-after-free.cocci

[Peter Maydell]

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

Re: [PATCH 1/6] scripts/coccinelle: add error-use-after-free.cocci

[Markus Armbruster]

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.

Re: [PATCH 1/6] scripts/coccinelle: add error-use-after-free.cocci

[Peter Maydell]

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

Re: [PATCH 1/6] scripts/coccinelle: add error-use-after-free.cocci

[Markus Armbruster]

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.

Re: [PATCH 1/6] scripts/coccinelle: add error-use-after-free.cocci

[Peter Maydell]

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

Re: [PATCH 1/6] scripts/coccinelle: add error-use-after-free.cocci

[Markus Armbruster]

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.

Re: [PATCH 1/6] scripts/coccinelle: add error-use-after-free.cocci

[Peter Maydell]

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

Re: [PATCH 1/6] scripts/coccinelle: add error-use-after-free.cocci

[Markus Armbruster]

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.

[...]

Re: [PATCH for-5.0 0/6] Several error use-after-free

[Markus Armbruster]

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

> Hi all!
>
> I accidentally found use-after-free of local_err in mirror, and decided
> to search for similar cases with help of small coccinelle script
> (patch 01). Happily, there no many cases.
>
> Better to fix zero Error* pointer after each freeing everywhere, but
> this is too much for 5.0 and most of these cases will be covered by
> error-auto-propagation series.
>
> Note also, that there are still a lot of use-after-free cases possible
> when error is not local variable but field of some structure, shared by
> several functions.

I queued the part that hasn't been merged.  Thanks!