On Tue, Sep 29, 2020 at 6:22 AM Eduardo Habkost <ehabk...@redhat.com> wrote:
>
> On Tue, Sep 29, 2020 at 01:55:35PM +0100, Peter Maydell wrote:
> > On Sun, 27 Sep 2020 at 15:00, Alistair Francis <alistair.fran...@wdc.com>
> > wrote:
> > >
> > > Reported-by: Eduardo Habkost <ehabk...@redhat.com>
> > > Signed-off-by: Alistair Francis <alistair.fran...@wdc.com>
> > > Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
> > > Message-Id:
> > > <4cf1beb7dafb9143c261d266557d3173bf160524.1598376594.git.alistair.fran...@wdc.com>
> > > ---
> > > @@ -269,13 +258,18 @@ static RegisterInfoArray
> > > *register_init_block(DeviceState *owner,
> > > int index = rae[i].addr / data_size;
> > > RegisterInfo *r = &ri[index];
> > >
> > > - *r = (RegisterInfo) {
> > > - .data = data + data_size * index,
> > > - .data_size = data_size,
> > > - .access = &rae[i],
> > > - .opaque = owner,
> > > - };
> > > - register_init(r);
> > > + if (data + data_size * index == 0 || !&rae[i]) {
> > > + continue;
> >
> > Coverity thinks (CID 1432800) that this is dead code, because
> > "data + data_size * index" can never be NULL[*]. What was this
> > intending to test for ? (maybe data == NULL? Missing dereference
> > operator ?)
>
> I believe the original check in the old register_init() function
> were just to make the function more flexible by allowing NULL
> arguments, but it was always unnecessary. We have 4 callers of
> register_init_block*() and neither rae or data are NULL on those
> calls.
In this case *data is an array, I guess the idea was to try and catch
if somehow a point in the array was NULL?
I'll send a patch to remove the check.
Alistair
>
> >
> > [*] The C spec is quite strict about what valid pointer arithmetic
> > is; in particular adding to a NULL pointer is undefined behaviour,
> > and pointer arithmetic that overflows and wraps around is
> > undefined behaviour, so there's no way to get a 0 result from
> > "ptr + offset" without the expression being UB.
> >
> > thanks
> > -- PMM
> >
>
> --
> Eduardo
>
