>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
g_malloc() is preferred more than g_try_* functions, which return NULL on error,
when the size of the requested allocation is small.
This is because allocating few bytes should not be a problem in a healthy
system.
Otherwise, the system is already in a critical state.
Plan to delete null checks after g malloc().
发件人: Peter Maydell
发送时间: 2023年7月25日 17:35
收件人: Michael Tokarev
抄送: dinglimin; richard.hender...@linaro.org; qemu-devel@nongnu.org
主题: Re: [PATCH] semihosting/uaccess.c: Replaced a malloc call with g_malloc.
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
