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 <address@hidden>
> > ---
> >   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