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

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

[dinglimin]

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>
---
 semihosting/uaccess.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/semihosting/uaccess.c b/semihosting/uaccess.c
index 8018828069..8f2e6f63ee 100644
--- a/semihosting/uaccess.c
+++ b/semihosting/uaccess.c
@@ -14,11 +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);
-            p = NULL;
+            g_free(p);
         }
     }
     return p;
-- 
2.30.0.windows.2

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

[Michael Tokarev]

25.07.2023 11:06, dinglimin wrote:
> Replaced a call to malloc() and its respective call to free() with g_malloc() and g_free().
...
>   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);
> -            p = NULL;
> +            g_free(p);
>           }
>       }
>       return p;

This is definitely wrong.

Hint: what this function will return if cpu_memory_rw_debug() fails?

/mjt

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

[dinglimin]

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;
         }
     }
-- 
2.30.0.windows.2

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

[Michael Tokarev]

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).

lock_user/unlock_user (which #defines to softmmu_lock_user/
softmmu_unlock_user in softmmu mode) is used a *lot*.

/mjt

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

[Peter Maydell]

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

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

[dinglimin]

[-- 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 --]

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

[Peter Maydell]

(Something went wrong with the quoting in your email. I've
fixed it up.)

On Wed, 26 Jul 2023 at 05:38, <dinglimin@cmss.chinamobile.com> wrote:
> Peter Maydell wrote:
> > 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().

> 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.

This is true. But in this particular case we cannot be sure
that the size of the allocation is small, because the size
is controlled by the guest. So we want g_try_malloc().

thanks
-- PMM

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

[Richard Henderson]

On 7/26/23 02:43, Peter Maydell wrote:
> (Something went wrong with the quoting in your email. I've
> fixed it up.)
> 
> On Wed, 26 Jul 2023 at 05:38, <dinglimin@cmss.chinamobile.com> wrote:
>> Peter Maydell wrote:
>>> 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().
> 
>> 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.
> 
> This is true. But in this particular case we cannot be sure
> that the size of the allocation is small, because the size
> is controlled by the guest. So we want g_try_malloc().

And why do we want to use g_try_malloc instead of just sticking with malloc?

I see no reason to change anything at all here.


r~

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

[Peter Maydell]

On Wed, 26 Jul 2023 at 16:21, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 7/26/23 02:43, Peter Maydell wrote:
> > (Something went wrong with the quoting in your email. I've
> > fixed it up.)
> >
> > On Wed, 26 Jul 2023 at 05:38, <dinglimin@cmss.chinamobile.com> wrote:
> >> Peter Maydell wrote:
> >>> 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().
> >
> >> 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.
> >
> > This is true. But in this particular case we cannot be sure
> > that the size of the allocation is small, because the size
> > is controlled by the guest. So we want g_try_malloc().
>
> And why do we want to use g_try_malloc instead of just sticking with malloc?

The only real reason is just consistency -- the project uses
the glib malloc wrappers, and in theory any use of raw
malloc() ought to be either:
 * something that's third party library code (eg libdecnumber)
 * because it's going into a standalone program that doesn't
   link against glib
 * for a special case reason which is documented in a
   nearby comment (eg because the memory is going to be
   passed to some library which documents that it will assume
   it can free() the memory)

We're down to very few uses of malloc() that don't fall
into one of those cases:

bsd-user/elfload.c
a few uses in disas/ (m68k, sparc, nios2)
hw/audio/fmopl.c
semihosting/uaccess.c
target/xtensa/translate.c
contrib/elf2dmp/
(and maybe tests/qtest/fuzz/qtest_wrappers.c : I'm not
sure what environment that gets built in.)

The main reason we get patches like this is that the
"bite sized tasks" list at
https://wiki.qemu.org/Contribute/BiteSizedTasks
has an entry for "convert malloc uses to g_new or similar".
The trouble with that is that almost all of the low-hanging
fruit was converted a long time ago, so the remainder
are getting increasingly tricky to analyse and increasingly
less worthwhile...

thanks
-- PMM

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

[Daniel P. Berrangé]

On Thu, Jul 27, 2023 at 03:56:23PM +0100, Peter Maydell wrote:
> On Wed, 26 Jul 2023 at 16:21, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> >
> > On 7/26/23 02:43, Peter Maydell wrote:
> > > (Something went wrong with the quoting in your email. I've
> > > fixed it up.)
> > >
> > > On Wed, 26 Jul 2023 at 05:38, <dinglimin@cmss.chinamobile.com> wrote:
> > >> Peter Maydell wrote:
> > >>> 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().
> > >
> > >> 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.
> > >
> > > This is true. But in this particular case we cannot be sure
> > > that the size of the allocation is small, because the size
> > > is controlled by the guest. So we want g_try_malloc().
> >
> > And why do we want to use g_try_malloc instead of just sticking with malloc?
> 
> The only real reason is just consistency

I think it is slightly stronger than that.

By using g_try_malloc we make it explicit that this scenario is
expecting the allocation to fail and needs to handle that.

If we use plain 'malloc' it isn't clear whether we genuinely expect
the allocation to fail, or someone just blindly checked malloc
return value out of habit, because they didn't realize QEMU wants
abort-on-OOM behaviour most of the time.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Richard Henderson]

On 7/27/23 08:04, Daniel P. Berrangé wrote:
> On Thu, Jul 27, 2023 at 03:56:23PM +0100, Peter Maydell wrote:
>> On Wed, 26 Jul 2023 at 16:21, Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>>>
>>> On 7/26/23 02:43, Peter Maydell wrote:
>>>> (Something went wrong with the quoting in your email. I've
>>>> fixed it up.)
>>>>
>>>> On Wed, 26 Jul 2023 at 05:38, <dinglimin@cmss.chinamobile.com> wrote:
>>>>> Peter Maydell wrote:
>>>>>> 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().
>>>>
>>>>> 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.
>>>>
>>>> This is true. But in this particular case we cannot be sure
>>>> that the size of the allocation is small, because the size
>>>> is controlled by the guest. So we want g_try_malloc().
>>>
>>> And why do we want to use g_try_malloc instead of just sticking with malloc?
>>
>> The only real reason is just consistency
> 
> I think it is slightly stronger than that.
> 
> By using g_try_malloc we make it explicit that this scenario is
> expecting the allocation to fail and needs to handle that.
> 
> If we use plain 'malloc' it isn't clear whether we genuinely expect
> the allocation to fail, or someone just blindly checked malloc
> return value out of habit, because they didn't realize QEMU wants
> abort-on-OOM behaviour most of the time.

Ok, fair enough.


r~

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

[dinglimin]

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>

v4 -> V5:Use g_try_malloc() instead of malloc()
V3 -> V4:Delete null checks after g malloc().
V2 -> V3:softmmu_unlock_user changes free to g free.
V1 -> V2:if cpu_memory_rw_debug failed, still need to set p=NULL

Signed-off-by: dinglimin <dinglimin@cmss.chinamobile.com>
---
 semihosting/uaccess.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/semihosting/uaccess.c b/semihosting/uaccess.c
index 8018828069..35fdcd69db 100644
--- a/semihosting/uaccess.c
+++ b/semihosting/uaccess.c
@@ -14,13 +14,20 @@
 void *softmmu_lock_user(CPUArchState *env, target_ulong addr,
                         target_ulong len, bool copy)
 {
-    void *p = malloc(len);
-    if (p && copy) {
+    void *p = g_try_malloc(len);
+
+    if (!p) {
+        p = NULL;
+        return p;
+    }
+
+    if (copy) {
         if (cpu_memory_rw_debug(env_cpu(env), addr, p, len, 0)) {
-            free(p);
+            g_free(p);
             p = NULL;
         }
     }
+
     return p;
 }
 
@@ -87,5 +94,5 @@ void softmmu_unlock_user(CPUArchState *env, void *p,
     if (len) {
         cpu_memory_rw_debug(env_cpu(env), addr, p, len, 1);
     }
-    free(p);
+    g_free(p);
 }
-- 
2.30.0.windows.2

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

[Peter Maydell]

On Fri, 28 Jul 2023 at 06:13, dinglimin <dinglimin@cmss.chinamobile.com> 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>
>
> v4 -> V5:Use g_try_malloc() instead of malloc()
> V3 -> V4:Delete null checks after g malloc().
> V2 -> V3:softmmu_unlock_user changes free to g free.
> V1 -> V2:if cpu_memory_rw_debug failed, still need to set p=NULL
>
> Signed-off-by: dinglimin <dinglimin@cmss.chinamobile.com>
> ---
>  semihosting/uaccess.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/semihosting/uaccess.c b/semihosting/uaccess.c
> index 8018828069..35fdcd69db 100644
> --- a/semihosting/uaccess.c
> +++ b/semihosting/uaccess.c
> @@ -14,13 +14,20 @@
>  void *softmmu_lock_user(CPUArchState *env, target_ulong addr,
>                          target_ulong len, bool copy)
>  {
> -    void *p = malloc(len);
> -    if (p && copy) {
> +    void *p = g_try_malloc(len);
> +
> +    if (!p) {
> +        p = NULL;

This doesn't make sense -- if (!p) means p is already NULL,
so you don't need to set it to NULL.

> +        return p;
> +    }

This patch should just replace malloc() with
g_try_malloc() and free() with g_free(). You don't need to
change any of the rest of the logic in the function.

> +
> +    if (copy) {
>          if (cpu_memory_rw_debug(env_cpu(env), addr, p, len, 0)) {
> -            free(p);
> +            g_free(p);
>              p = NULL;
>          }
>      }
> +
>      return p;
>  }
>
> @@ -87,5 +94,5 @@ void softmmu_unlock_user(CPUArchState *env, void *p,
>      if (len) {
>          cpu_memory_rw_debug(env_cpu(env), addr, p, len, 1);
>      }
> -    free(p);
> +    g_free(p);
>  }
> --
> 2.30.0.windows.2

thanks
-- PMM

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

[dinglimin]

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>

v4 -> V5:Use g_try_malloc() instead of malloc()
V3 -> V4:Delete null checks after g malloc().
V2 -> V3:softmmu_unlock_user changes free to g free.
V1 -> V2:if cpu_memory_rw_debug failed, still need to set p=NULL

Signed-off-by: dinglimin <dinglimin@cmss.chinamobile.com>
---
 semihosting/uaccess.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/semihosting/uaccess.c b/semihosting/uaccess.c
index 8018828069..980138ea69 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_try_malloc(len);
     if (p && copy) {
         if (cpu_memory_rw_debug(env_cpu(env), addr, p, len, 0)) {
-            free(p);
+            g_free(p);
             p = NULL;
         }
     }
@@ -87,5 +87,5 @@ void softmmu_unlock_user(CPUArchState *env, void *p,
     if (len) {
         cpu_memory_rw_debug(env_cpu(env), addr, p, len, 1);
     }
-    free(p);
+    g_free(p);
 }
-- 
2.30.0.windows.2

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

[Peter Maydell]

On Fri, 28 Jul 2023 at 11:50, dinglimin <dinglimin@cmss.chinamobile.com> 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>
>
> v4 -> V5:Use g_try_malloc() instead of malloc()
> V3 -> V4:Delete null checks after g malloc().
> V2 -> V3:softmmu_unlock_user changes free to g free.
> V1 -> V2:if cpu_memory_rw_debug failed, still need to set p=NULL
>
> Signed-off-by: dinglimin <dinglimin@cmss.chinamobile.com>
> ---

Thanks for this patch, and for working through the code
review process with us on this!

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

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

[Peter Maydell]

On Thu, 27 Jul 2023 at 15:56, Peter Maydell <peter.maydell@linaro.org> wrote:
> The only real reason is just consistency -- the project uses
> the glib malloc wrappers, and in theory any use of raw
> malloc() ought to be either:
>  * something that's third party library code (eg libdecnumber)
>  * because it's going into a standalone program that doesn't
>    link against glib
>  * for a special case reason which is documented in a
>    nearby comment (eg because the memory is going to be
>    passed to some library which documents that it will assume
>    it can free() the memory)

I wrote up a gitlab bite-sized-task issue for the remaining
conversions which goes into a bit more detail about some
of the pitfalls to watch out for, and made the bitesizedtasks
wiki page link to that:
 https://gitlab.com/qemu-project/qemu/-/issues/1798

thanks
-- PMM

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

[dinglimin]

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>

V3 -> V4:Delete null checks after g malloc().
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.

V2 -> V3:softmmu_unlock_user changes free to g free.
V1 -> V2:if cpu_memory_rw_debug failed, still need to set p=NULL

Signed-off-by: dinglimin <dinglimin@cmss.chinamobile.com>
---
 semihosting/uaccess.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/semihosting/uaccess.c b/semihosting/uaccess.c
index 8018828069..81a0f80e9f 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;
         }
     }
@@ -87,5 +87,5 @@ void softmmu_unlock_user(CPUArchState *env, void *p,
     if (len) {
         cpu_memory_rw_debug(env_cpu(env), addr, p, len, 1);
     }
-    free(p);
+    g_free(p);
 }
-- 
2.30.0.windows.2

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

[dinglimin]

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>

V2 -> V3:softmmu_unlock_user changes free to g free.
V1 -> V2:if cpu_memory_rw_debug failed, still need to set p=NULL
---
 semihosting/uaccess.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/semihosting/uaccess.c b/semihosting/uaccess.c
index 8018828069..81a0f80e9f 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;
         }
     }
@@ -87,5 +87,5 @@ void softmmu_unlock_user(CPUArchState *env, void *p,
     if (len) {
         cpu_memory_rw_debug(env_cpu(env), addr, p, len, 1);
     }
-    free(p);
+    g_free(p);
 }
-- 
2.30.0.windows.2

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

[dinglimin]

Signed-off-by: dinglimin <dinglimin@cmss.chinamobile.com>
---
 semihosting/uaccess.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/semihosting/uaccess.c b/semihosting/uaccess.c
index dc587d73bc..7788ead9b2 100644
--- a/semihosting/uaccess.c
+++ b/semihosting/uaccess.c
@@ -14,10 +14,10 @@
 void *uaccess_lock_user(CPUArchState *env, target_ulong addr,
                         target_ulong len, bool copy)
 {
-    void *p = malloc(len);
+    void *p = g_try_malloc(len);
     if (p && copy) {
         if (cpu_memory_rw_debug(env_cpu(env), addr, p, len, 0)) {
-            free(p);
+            g_free(p);
             p = NULL;
         }
     }
@@ -87,5 +87,5 @@ void uaccess_unlock_user(CPUArchState *env, void *p,
     if (len) {
         cpu_memory_rw_debug(env_cpu(env), addr, p, len, 1);
     }
-    free(p);
+    g_free(p);
 }
-- 
2.30.0.windows.2

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

[Philippe Mathieu-Daudé]

Hi,

On 26/2/24 10:06, dinglimin wrote:
> Signed-off-by: dinglimin <dinglimin@cmss.chinamobile.com>
> ---
>   semihosting/uaccess.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/semihosting/uaccess.c b/semihosting/uaccess.c
> index dc587d73bc..7788ead9b2 100644
> --- a/semihosting/uaccess.c
> +++ b/semihosting/uaccess.c
> @@ -14,10 +14,10 @@
>   void *uaccess_lock_user(CPUArchState *env, target_ulong addr,
>                           target_ulong len, bool copy)
>   {
> -    void *p = malloc(len);
> +    void *p = g_try_malloc(len);
>       if (p && copy) {
>           if (cpu_memory_rw_debug(env_cpu(env), addr, p, len, 0)) {
> -            free(p);
> +            g_free(p);
>               p = NULL;
>           }
>       }

This seems dangerous, now all users of uaccess_lock_user() must
use g_free(), in particular lock_user_string() which is used more
than a hundred of times:

$ git grep -w lock_user_string | wc -l
      116

> @@ -87,5 +87,5 @@ void uaccess_unlock_user(CPUArchState *env, void *p,
>       if (len) {
>           cpu_memory_rw_debug(env_cpu(env), addr, p, len, 1);
>       }
> -    free(p);
> +    g_free(p);
>   }

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

[Daniel P. Berrangé]

On Mon, Feb 26, 2024 at 10:48:14AM +0100, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> On 26/2/24 10:06, dinglimin wrote:
> > Signed-off-by: dinglimin <dinglimin@cmss.chinamobile.com>
> > ---
> >   semihosting/uaccess.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/semihosting/uaccess.c b/semihosting/uaccess.c
> > index dc587d73bc..7788ead9b2 100644
> > --- a/semihosting/uaccess.c
> > +++ b/semihosting/uaccess.c
> > @@ -14,10 +14,10 @@
> >   void *uaccess_lock_user(CPUArchState *env, target_ulong addr,
> >                           target_ulong len, bool copy)
> >   {
> > -    void *p = malloc(len);
> > +    void *p = g_try_malloc(len);
> >       if (p && copy) {
> >           if (cpu_memory_rw_debug(env_cpu(env), addr, p, len, 0)) {
> > -            free(p);
> > +            g_free(p);
> >               p = NULL;
> >           }
> >       }
> 
> This seems dangerous, now all users of uaccess_lock_user() must
> use g_free(), in particular lock_user_string() which is used more
> than a hundred of times:

This is not true for many years now.

GLib is hardcoded to always use the system allocator, so g_malloc
can be freely mixed with free, and vica-verca.

Using 'g_free' is stylistically preferred, but not functionally
required.

> 
> $ git grep -w lock_user_string | wc -l
>      116
> 
> > @@ -87,5 +87,5 @@ void uaccess_unlock_user(CPUArchState *env, void *p,
> >       if (len) {
> >           cpu_memory_rw_debug(env_cpu(env), addr, p, len, 1);
> >       }
> > -    free(p);
> > +    g_free(p);
> >   }
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Peter Maydell]

On Mon, 26 Feb 2024 at 09:49, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi,
>
> On 26/2/24 10:06, dinglimin wrote:
> > Signed-off-by: dinglimin <dinglimin@cmss.chinamobile.com>
> > ---
> >   semihosting/uaccess.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/semihosting/uaccess.c b/semihosting/uaccess.c
> > index dc587d73bc..7788ead9b2 100644
> > --- a/semihosting/uaccess.c
> > +++ b/semihosting/uaccess.c
> > @@ -14,10 +14,10 @@
> >   void *uaccess_lock_user(CPUArchState *env, target_ulong addr,
> >                           target_ulong len, bool copy)
> >   {
> > -    void *p = malloc(len);
> > +    void *p = g_try_malloc(len);
> >       if (p && copy) {
> >           if (cpu_memory_rw_debug(env_cpu(env), addr, p, len, 0)) {
> > -            free(p);
> > +            g_free(p);
> >               p = NULL;
> >           }
> >       }
>
> This seems dangerous, now all users of uaccess_lock_user() must
> use g_free(), in particular lock_user_string() which is used more
> than a hundred of times:

Users of lock_user_string() and other lock_user() functions are
supposed to unlock via unlock_user(); if they directly call either
free() or g_free() they have a bug.

-- PMM

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

[Zhao Liu]

On Mon, Feb 26, 2024 at 05:06:28PM +0800, dinglimin wrote:
> Date: Mon, 26 Feb 2024 17:06:28 +0800
> From: dinglimin <dinglimin@cmss.chinamobile.com>
> Subject: [PATCH] semihosting/uaccess.c: Replaced a malloc call with g_malloc
> X-Mailer: git-send-email 2.30.0.windows.2
> 
> Signed-off-by: dinglimin <dinglimin@cmss.chinamobile.com>
> ---
>  semihosting/uaccess.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Also cc Michael and qemu-trivial@nongnu.org.
I understand the simple cleanup can cc qemu-trivial@nongnu.org. ;-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

> 
> diff --git a/semihosting/uaccess.c b/semihosting/uaccess.c
> index dc587d73bc..7788ead9b2 100644
> --- a/semihosting/uaccess.c
> +++ b/semihosting/uaccess.c
> @@ -14,10 +14,10 @@
>  void *uaccess_lock_user(CPUArchState *env, target_ulong addr,
>                          target_ulong len, bool copy)
>  {
> -    void *p = malloc(len);
> +    void *p = g_try_malloc(len);
>      if (p && copy) {
>          if (cpu_memory_rw_debug(env_cpu(env), addr, p, len, 0)) {
> -            free(p);
> +            g_free(p);
>              p = NULL;
>          }
>      }
> @@ -87,5 +87,5 @@ void uaccess_unlock_user(CPUArchState *env, void *p,
>      if (len) {
>          cpu_memory_rw_debug(env_cpu(env), addr, p, len, 1);
>      }
> -    free(p);
> +    g_free(p);
>  }
> -- 
> 2.30.0.windows.2