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

[Markus Armbruster]

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