In framebuffer_update_display(), Coverity complains because we
multiply two values of type 'int' (which will be done as a 32x32
multiply and so in theory might overflow) and then add the result to
a ram_addr_t, which can be 64 bits.

4GB framebuffers are not plausible anyway, but keep Coverity happy
by adding casts which force these multiplies to be done as 64x64.

Coverity: CID 1487248
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This is one of those ones where I'm on the fence about sticking
in the cast vs just marking it a false-positive.
---
 hw/display/framebuffer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/display/framebuffer.c b/hw/display/framebuffer.c
index 4485aa335bb..b4296e8a33e 100644
--- a/hw/display/framebuffer.c
+++ b/hw/display/framebuffer.c
@@ -95,9 +95,9 @@ void framebuffer_update_display(
     }
     first = -1;
 
-    addr += i * src_width;
-    src += i * src_width;
-    dest += i * dest_row_pitch;
+    addr += (uint64_t)i * src_width;
+    src += (uint64_t)i * src_width;
+    dest += (uint64_t)i * dest_row_pitch;
 
     snap = memory_region_snapshot_and_clear_dirty(mem, addr, src_width * rows,
                                                   DIRTY_MEMORY_VGA);
-- 
2.43.0

Ping -- any opinions/review about this one?

-- PMM

On Thu, 10 Jul 2025 at 18:43, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In framebuffer_update_display(), Coverity complains because we
> multiply two values of type 'int' (which will be done as a 32x32
> multiply and so in theory might overflow) and then add the result to
> a ram_addr_t, which can be 64 bits.
>
> 4GB framebuffers are not plausible anyway, but keep Coverity happy
> by adding casts which force these multiplies to be done as 64x64.
>
> Coverity: CID 1487248
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This is one of those ones where I'm on the fence about sticking
> in the cast vs just marking it a false-positive.
> ---
>  hw/display/framebuffer.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/display/framebuffer.c b/hw/display/framebuffer.c
> index 4485aa335bb..b4296e8a33e 100644
> --- a/hw/display/framebuffer.c
> +++ b/hw/display/framebuffer.c
> @@ -95,9 +95,9 @@ void framebuffer_update_display(
>      }
>      first = -1;
>
> -    addr += i * src_width;
> -    src += i * src_width;
> -    dest += i * dest_row_pitch;
> +    addr += (uint64_t)i * src_width;
> +    src += (uint64_t)i * src_width;
> +    dest += (uint64_t)i * dest_row_pitch;
>
>      snap = memory_region_snapshot_and_clear_dirty(mem, addr, src_width * rows,
>                                                    DIRTY_MEMORY_VGA);

On Mon, Jul 21, 2025 at 4:58 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Ping -- any opinions/review about this one?
>
> -- PMM
>
> On Thu, 10 Jul 2025 at 18:43, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > In framebuffer_update_display(), Coverity complains because we
> > multiply two values of type 'int' (which will be done as a 32x32
> > multiply and so in theory might overflow) and then add the result to
> > a ram_addr_t, which can be 64 bits.
> >
> > 4GB framebuffers are not plausible anyway, but keep Coverity happy
> > by adding casts which force these multiplies to be done as 64x64.
> >
> > Coverity: CID 1487248
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > This is one of those ones where I'm on the fence about sticking
> > in the cast vs just marking it a false-positive.
> > ---
> >  hw/display/framebuffer.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/display/framebuffer.c b/hw/display/framebuffer.c
> > index 4485aa335bb..b4296e8a33e 100644
> > --- a/hw/display/framebuffer.c
> > +++ b/hw/display/framebuffer.c
> > @@ -95,9 +95,9 @@ void framebuffer_update_display(
> >      }
> >      first = -1;
> >
> > -    addr += i * src_width;
> > -    src += i * src_width;
> > -    dest += i * dest_row_pitch;
> > +    addr += (uint64_t)i * src_width;
> > +    src += (uint64_t)i * src_width;
> > +    dest += (uint64_t)i * dest_row_pitch;
> >
> >      snap = memory_region_snapshot_and_clear_dirty(mem, addr, src_width * rows,
> >                                                    DIRTY_MEMORY_VGA);
>

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>