回复: [PATCH] semihosting/uaccess.c: Replaced a malloc call with g_malloc.

[<dinglimin@cmss.chinamobile.com>]

[-- Attachment #1: Type: text/plain, Size: 4989 bytes --]

>On Tue, 25 Jul 2023 at 10:13, Michael Tokarev <mjt@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 <dinglimin@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.henderson@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 <mjt@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 <dinglimin@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


[-- Attachment #2: Type: text/html, Size: 13679 bytes --]