Re: [PATCH v1] memory: assert MemoryRegionOps callbacks are defined

["Alex Bennée" <alex.bennee@linaro.org>]

P J P <ppandit@redhat.com> writes:

> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> When registering a MemoryRegionOps object, assert that its
> read/write callback methods are defined. This avoids potential
> guest crash via a NULL pointer dereference.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  memory.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> Update v1: add assert while registering MemoryRegionOps
>   -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg05187.html
>
> diff --git a/memory.c b/memory.c
> index 91ceaf9fcf..6e94fd5958 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1495,6 +1495,9 @@ void memory_region_init_io(MemoryRegion *mr,
>                             const char *name,
>                             uint64_t size)
>  {
> +    assert(ops);
> +    assert(ops->read);
> +    assert(ops->write);

If you look at memory_region_dispatch_write you can see that
mr->ops->write being empty is acceptable because it implies
mr->ops->write_with_attrs is set instead. I think the same is true for
read so I think you need something more like:

     assert(ops->read || ops->read_with_attrs);
     assert(ops->write || ops->write_with_attrs);
     

>      memory_region_init(mr, owner, name, size);
>      mr->ops = ops ? ops : &unassigned_mem_ops;
>      mr->opaque = opaque;
> @@ -1674,6 +1677,8 @@ void memory_region_init_rom_device_nomigrate(MemoryRegion *mr,
>  {
>      Error *err = NULL;
>      assert(ops);
> +    assert(ops->read);
> +    assert(ops->write);

Do ROM devices need a ->write function?

Also doesn't this break a load of running stuff without fixes for all
the various missing bits? How far does make check-acceptance get?

>      memory_region_init(mr, owner, name, size);
>      mr->ops = ops;
>      mr->opaque = opaque;


-- 
Alex Bennée