On Tue, 25 Jul 2023 at 10:13, Michael Tokarev <m...@tls.msk.ru> wrote:
>
> 25.07.2023 12:00, dinglimin wrote:
> > Replaced a call to malloc() and its respective call to free() with
> > g_malloc() and g_free().
> >
> > Signed-off-by: dinglimin <dingli...@cmss.chinamobile.com>
> >
> > V1 -> V2:if cpu_memory_rw_debug failed, still need to set p=NULL
> > ---
> > semihosting/uaccess.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/semihosting/uaccess.c b/semihosting/uaccess.c
> > index 8018828069..2ac754cdb6 100644
> > --- a/semihosting/uaccess.c
> > +++ b/semihosting/uaccess.c
> > @@ -14,10 +14,10 @@
> > void *softmmu_lock_user(CPUArchState *env, target_ulong addr,
> > target_ulong len, bool copy)
> > {
> > - void *p = malloc(len);
> > + void *p = g_malloc(len);
> > if (p && copy) {
> > if (cpu_memory_rw_debug(env_cpu(env), addr, p, len, 0)) {
> > - free(p);
> > + g_free(p);
> > p = NULL;
> > }
> > }
>
> Ok, that was the obvious part. Now a next one, also obvious.
>
> You changed lock_user to use g_malloc(), but unlock_user
> still uses free() instead of g_free(). At the very least
> the other one needs to be changed too. And I'd say the callers
> should be analyzed to ensure they don't free() the result
> (they should not, think it is a bug if they do).
We can be pretty sure the callers don't free() the returned
value, because the calling code is also used in user-mode,
where the lock/unlock implementation is entirely different
and calling free() on the pointer will not work.
> lock_user/unlock_user (which #defines to softmmu_lock_user/
> softmmu_unlock_user in softmmu mode) is used a *lot*.
The third part here, is that g_malloc() does not ever
fail -- it will abort() on out of memory. However
the code here is still handling g_malloc() returning NULL.
The equivalent for "we expect this might fail" (which we want
here, because the guest is passing us the length of memory
to try to allocate) is g_try_malloc().
thanks
-- PMM
