hw/display/tcx: use memmove instead of memcpy for overlapping blit memory regions

[PATCH] hw/display/tcx: use memmove instead of memcpy for overlapping blit memory regions

Posted by Николай Зорин 1 week, 4 days ago

SWSA#34e3af79-c6c2-4ad2-986f-313c1ce28e5a

In `tcx_blit_writel()`, memory copying occurs within the same video RAM
buffers (`vram` and `vram24`) using source and destination offsets that
can potentially overlap during blit operations.

Using `memcpy()` on overlapping memory regions triggers undefined behavior
according to the C standard. Depending on compiler optimizations, this
can lead to graphical artifacts, memory corruption, or unexpected
behavior in the guest system.

Fix this by replacing `memcpy()` with `memmove()`, which explicitly
guarantees safe and correct behavior when memory regions overlap.

Signed-off-by: Nikolay N Zorin <zorin@swemel.ru>
---
  hw/display/tcx.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 2cfc1e8f..fb48a199 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -587,9 +587,9 @@ static void tcx_blit_writel(void *opaque, hwaddr addr,
                  }
              }
          } else {
-            memcpy(&s->vram[addr], &s->vram[adsr], len);
+            memmove(&s->vram[addr], &s->vram[adsr], len);
              if (s->depth == 24) {
-                memcpy(&s->vram24[addr], &s->vram24[adsr], len * 4);
+                memmove(&s->vram24[addr], &s->vram24[adsr], len * 4);
              }
          }
          tcx_set_dirty(s, addr, len);
-- 
2.43.0

Re: [PATCH] hw/display/tcx: use memmove instead of memcpy for overlapping blit memory regions

Posted by Mark Cave-Ayland 1 week, 1 day ago

On 15/09/2026 17:20, Николай Зорин wrote:

> SWSA#34e3af79-c6c2-4ad2-986f-313c1ce28e5a

I guess this is from something else?

> In `tcx_blit_writel()`, memory copying occurs within the same video RAM
> buffers (`vram` and `vram24`) using source and destination offsets that
> can potentially overlap during blit operations.
> 
> Using `memcpy()` on overlapping memory regions triggers undefined behavior
> according to the C standard. Depending on compiler optimizations, this
> can lead to graphical artifacts, memory corruption, or unexpected
> behavior in the guest system.

I would hope that this is just graphical artifacts given that this is just video RAM...

> Fix this by replacing `memcpy()` with `memmove()`, which explicitly
> guarantees safe and correct behavior when memory regions overlap.

Do you have a specific test case? I can see that this would make sense in some cases, 
e.g. sideways scrolling.

> Signed-off-by: Nikolay N Zorin <zorin@swemel.ru>
> ---
>   hw/display/tcx.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/display/tcx.c b/hw/display/tcx.c
> index 2cfc1e8f..fb48a199 100644
> --- a/hw/display/tcx.c
> +++ b/hw/display/tcx.c
> @@ -587,9 +587,9 @@ static void tcx_blit_writel(void *opaque, hwaddr addr,
>                   }
>               }
>           } else {
> -            memcpy(&s->vram[addr], &s->vram[adsr], len);
> +            memmove(&s->vram[addr], &s->vram[adsr], len);
>               if (s->depth == 24) {
> -                memcpy(&s->vram24[addr], &s->vram24[adsr], len * 4);
> +                memmove(&s->vram24[addr], &s->vram24[adsr], len * 4);
>               }
>           }
>           tcx_set_dirty(s, addr, len);

Please can you can send a v2 with the commit message updated, then if it looks good 
I'll queue it in my qemu-sparc branch.


ATB,

Mark.

Re: [PATCH] hw/display/tcx: use memmove instead of memcpy for overlapping blit memory regions

Posted by Michael S. Tsirkin 1 week, 4 days ago

thanks for the patch! some questions:

On Tue, Sep 15, 2026 at 07:20:46PM +0300, Николай Зорин wrote:
> SWSA#34e3af79-c6c2-4ad2-986f-313c1ce28e5a

what is this?

> In `tcx_blit_writel()`, memory copying occurs within the same video RAM
> buffers (`vram` and `vram24`) using source and destination offsets that
> can potentially overlap during blit operations.
> 
> Using `memcpy()` on overlapping memory regions triggers undefined behavior
> according to the C standard. Depending on compiler optimizations, this
> can lead to graphical artifacts, memory corruption, or unexpected
> behavior in the guest system.

was this actually observed? in which configuration? which guest?
why does the guest do such overlapping blit write?

it also seems unlikely to depend on optimizations - more likely
on libc.

> Fix this by replacing `memcpy()` with `memmove()`, which explicitly
> guarantees safe and correct behavior when memory regions overlap.
> 
> Signed-off-by: Nikolay N Zorin <zorin@swemel.ru>
> ---
>  hw/display/tcx.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/display/tcx.c b/hw/display/tcx.c
> index 2cfc1e8f..fb48a199 100644
> --- a/hw/display/tcx.c
> +++ b/hw/display/tcx.c
> @@ -587,9 +587,9 @@ static void tcx_blit_writel(void *opaque, hwaddr addr,
>                  }
>              }
>          } else {
> -            memcpy(&s->vram[addr], &s->vram[adsr], len);
> +            memmove(&s->vram[addr], &s->vram[adsr], len);
>              if (s->depth == 24) {
> -                memcpy(&s->vram24[addr], &s->vram24[adsr], len * 4);
> +                memmove(&s->vram24[addr], &s->vram24[adsr], len * 4);
>              }
>          }
>          tcx_set_dirty(s, addr, len);
> -- 
> 2.43.0

Re: [PATCH] hw/display/tcx: use memmove instead of memcpy for overlapping blit memory regions

Posted by Daniel P. Berrangé 1 week, 4 days ago

On Tue, Sep 15, 2026 at 07:20:46PM +0300, Николай Зорин wrote:
> SWSA#34e3af79-c6c2-4ad2-986f-313c1ce28e5a

  ^^^^^^^^^  what is this ?

I'm guessing it is a tag you're using for tracking issues, but
if so, it should be removed before sending commits upstream.

> 
> In `tcx_blit_writel()`, memory copying occurs within the same video RAM
> buffers (`vram` and `vram24`) using source and destination offsets that
> can potentially overlap during blit operations.
> 
> Using `memcpy()` on overlapping memory regions triggers undefined behavior
> according to the C standard. Depending on compiler optimizations, this
> can lead to graphical artifacts, memory corruption, or unexpected
> behavior in the guest system.
> 
> Fix this by replacing `memcpy()` with `memmove()`, which explicitly
> guarantees safe and correct behavior when memory regions overlap.
> 
> Signed-off-by: Nikolay N Zorin <zorin@swemel.ru>
> ---
>  hw/display/tcx.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/display/tcx.c b/hw/display/tcx.c
> index 2cfc1e8f..fb48a199 100644
> --- a/hw/display/tcx.c
> +++ b/hw/display/tcx.c
> @@ -587,9 +587,9 @@ static void tcx_blit_writel(void *opaque, hwaddr addr,
>                  }
>              }
>          } else {
> -            memcpy(&s->vram[addr], &s->vram[adsr], len);
> +            memmove(&s->vram[addr], &s->vram[adsr], len);
>              if (s->depth == 24) {
> -                memcpy(&s->vram24[addr], &s->vram24[adsr], len * 4);
> +                memmove(&s->vram24[addr], &s->vram24[adsr], len * 4);
>              }
>          }
>          tcx_set_dirty(s, addr, len);
> -- 
> 2.43.0
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH] hw/display/tcx: use memmove instead of memcpy for overlapping blit memory regions

Posted by Николай Зорин 1 week, 4 days ago

This is our tag. Okay, in the future I'll remove the "garbage" (anything 
that doesn't relate to the original).

15.09.2026 19:31, Daniel P. Berrangé пишет:
> I'm guessing it is a tag you're using for tracking issues, but
> if so, it should be removed before sending commits upstream.