Max Reitz <mre...@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
