| Message ID | 20240226090628.1986-1-dinglimin@cmss.chinamobile.com |
|---|---|
| State | New |
| Headers | show |
| Series | semihosting/uaccess.c: Replaced a malloc call with g_malloc | expand |
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); > }
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
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
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
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); }
Signed-off-by: dinglimin <dinglimin@cmss.chinamobile.com> --- semihosting/uaccess.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)