From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 315BDC001DE for ; Wed, 26 Jul 2023 04:38:28 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qOWH9-0002EN-16; Wed, 26 Jul 2023 00:37:27 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qOWH5-0002Du-U6 for qemu-devel@nongnu.org; Wed, 26 Jul 2023 00:37:23 -0400 Received: from cmccmta6.chinamobile.com ([111.22.67.139] helo=cmccmta2.chinamobile.com) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qOWH2-0001ad-FT for qemu-devel@nongnu.org; Wed, 26 Jul 2023 00:37:23 -0400 X-RM-TagInfo: emlType=0 X-RM-SPAM-FLAG: 00000000 Received: from spf.mail.chinamobile.com (unknown[10.188.0.87]) by rmmx-syy-dmz-app05-12005 (RichMail) with SMTP id 2ee564c0a2ed39b-1e486; Wed, 26 Jul 2023 12:37:12 +0800 (CST) X-RM-TRANSID: 2ee564c0a2ed39b-1e486 X-RM-TagInfo: emlType=0 X-RM-SPAM-FLAG: 00000000 Received: from [IPv6:::ffff:172.20.211.33] (unknown[223.108.79.99]) by rmsmtp-syy-appsvr05-12005 (RichMail) with SMTP id 2ee564c0a2f760a-62850; Wed, 26 Jul 2023 12:37:12 +0800 (CST) X-RM-TRANSID: 2ee564c0a2f760a-62850 MIME-Version: 1.0 To: Peter Maydell , Michael Tokarev Cc: "richard.henderson@linaro.org" , "qemu-devel@nongnu.org" From: Subject: =?utf-8?Q?=E5=9B=9E=E5=A4=8D:_[PATCH]_semihosting/uaccess.c:_Replaced_a_m?= =?utf-8?Q?alloc_call_with_g=5Fmalloc.?= Date: Wed, 26 Jul 2023 12:37:10 +0800 Importance: normal X-Priority: 3 In-Reply-To: References: <20230725080630.1083-1-dinglimin@cmss.chinamobile.com> <20230725090039.1271-1-dinglimin@cmss.chinamobile.com> <106cf02f-f746-e216-22be-8f7594028695@tls.msk.ru> Content-Type: multipart/alternative; boundary="_3DD5A187-4286-4919-9294-C9508DF89758_" Received-SPF: pass client-ip=111.22.67.139; envelope-from=dinglimin@cmss.chinamobile.com; helo=cmccmta2.chinamobile.com X-Spam_score_int: -13 X-Spam_score: -1.4 X-Spam_bar: - X-Spam_report: (-1.4 / 5.0 requ) BAYES_00=-1.9, DOS_BODY_HIGH_NO_MID=0.001, HTML_MESSAGE=0.001, MISSING_MID=0.497, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=no autolearn_force=no X-Spam_action: no action Message-Id: X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org --_3DD5A187-4286-4919-9294-C9508DF89758_ Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" >On Tue, 25 Jul 2023 at 10:13, Michael Tokarev 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 > > > > > > V1 -> V2:if cpu_memory_rw_debug failed, still need to set p=3DNULL > > > --- > > > 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 =3D malloc(len); > > > + void *p =3D g_malloc(len); > > > if (p && copy) { > > > if (cpu_memory_rw_debug(env_cpu(env), addr, p, len, 0)) { > > > - free(p); > > > + g_free(p); > > > p =3D 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. >=20 > > lock_user/unlock_user (which #defines to softmmu_lock_user/ > > softmmu_unlock_user in softmmu mode) is used a *lot*. >=20 > 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(). >=20 > thanks > -- PMM g_malloc() is preferred more than g_try_* functions, which return NULL on e= rror, when the size of the requested allocation is small.=20 This is because allocating few bytes should not be a problem in a healthy s= ystem.=20 Otherwise, the system is already in a critical state. Plan to delete null checks after g malloc(). =E5=8F=91=E4=BB=B6=E4=BA=BA: Peter Maydell =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2023=E5=B9=B47=E6=9C=8825=E6=97=A5 17= :35 =E6=94=B6=E4=BB=B6=E4=BA=BA: Michael Tokarev =E6=8A=84=E9=80=81: dinglimin; richard.henderson@linaro.org; qemu-devel@non= gnu.org =E4=B8=BB=E9=A2=98: Re: [PATCH] semihosting/uaccess.c: Replaced a malloc ca= ll with g_malloc. On Tue, 25 Jul 2023 at 10:13, Michael Tokarev wrote: > > 25.07.2023 12:00, dinglimin wrote: > > Replaced a call to malloc() and its respective call to free() with g_ma= lloc() and g_free(). > > > > Signed-off-by: dinglimin > > > > V1 -> V2:if cpu_memory_rw_debug failed, still need to set p=3DNULL > > --- > > 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 =3D malloc(len); > > + void *p =3D g_malloc(len); > > if (p && copy) { > > if (cpu_memory_rw_debug(env_cpu(env), addr, p, len, 0)) { > > - free(p); > > + g_free(p); > > p =3D 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 --_3DD5A187-4286-4919-9294-C9508DF89758_ Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset="utf-8"

>On Tue, 25 Ju= l 2023 at 10:13, Michael Tokarev <mjt@tls.msk.ru> wrote:

> >

> > 25.07.2023 12:00, dinglimin wrote:<= /p>

> > > Replaced a call t= o malloc() and its respective call to free() with g_malloc() and g_free().<= /span>

> > >

=

> > > Signed-off-by: dingl= imin <dinglimin@cmss.chinamobile.com>

= > > >

> > > V1 -> V2:if cpu_memory_rw_debug failed, still ne= ed to set p=3DNULL

> &= gt; > ---

> > &g= t;=C2=A0=C2=A0 semihosting/uaccess.c | 4 ++--

> > >=C2=A0=C2=A0 1 file changed, 2 insertion= s(+), 2 deletions(-)

>= > >

> > >= diff --git a/semihosting/uaccess.c b/semihosting/uaccess.c

> > > index 8018828069..2ac754c= db6 100644

> > >= --- a/semihosting/uaccess.c

> > > +++ b/semihosting/uaccess.c

> > > @@ -14,10 +14,10 @@

> > >=C2=A0=C2=A0 void *softmmu_lo= ck_user(CPUArchState *env, target_ulong addr,

> > >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 target_ulong len, bool copy)

> > >=C2=A0=C2=A0 {=

> > > -=C2=A0= =C2=A0=C2=A0 void *p =3D malloc(len);

> > > +=C2=A0=C2=A0=C2=A0 void *p =3D g_malloc(len);<= /span>

> > >=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 if (p && copy) {

> > >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 if (cpu_memory_rw_debug(env_cpu(env), addr, p, = len, 0)) {

> > >= -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 free(p= );

> > > +=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 g_free(p);

> > >=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 p = =3D NULL;

> > >= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }

>> >=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 }

> ><= /span>

>> Ok, that was the= obvious part.=C2=A0 Now a next one, also obvious.

> >

> > You changed lock_user to use g_malloc(), but unlock_us= er

> > still uses f= ree() instead of g_free().=C2=A0 At the very least

> > the other one needs to be changed too.= =C2=A0 And I'd say the callers

> > should be analyzed to ensure they don't free() the result

> (they should not, thi= nk it is a bug if they do).

> 

= > We can be pretty sure the callers don't free() the returned

=

> value, because the calling cod= e is also used in user-mode,

> where the lock/unlock implementation is entirely different<= /p>

> and calling free() on the p= ointer will not work.

>= ;

> > lock_user/un= lock_user (which #defines to softmmu_lock_user/

> > softmmu_unlock_user in softmmu mode) is us= ed a *lot*.

> <= /p>

> The third part here, is tha= t g_malloc() does not ever

> fail -- it will abort() on out of memory. However

> the code here is still handling g_mall= oc() returning NULL.

>= The equivalent for "we expect this might fail" (which we want

> here, because the gues= t 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,<= /p>

when the size of the requested = allocation=C2=A0 is small.

This is because allocating few bytes should not be a problem in a health= y system.

Otherwise, the= system is already in a critical state.

 

Plan to delete null checks after g malloc().

<= span lang=3DEN-US> 

 <= /span>

=E5=8F=91=E4=BB=B6=E4=BA=BA: Peter Maydell
=E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2023=E5=B9=B47=E6=9C=8825=E6=97=A5 17:35
=E6=94=B6=E4=BB=B6=E4=BA=BA= : Michael T= okarev
=E6=8A=84=E9=80=81: <= span lang=3DEN-US>dinglim= in; richard.henderson@l= inaro.org; qemu-devel@nongnu.o= rg
=E4=B8=BB=E9=A2=98: Re: [PATCH] semihosting/uaccess.c: Replaced a malloc call wit= h g_malloc.

 

On Tue, 25 Jul 2023 at 10:13, Michael Tok= arev <mjt@tls.msk.ru> wrote:

> 

> 25.07.2023 12:00, dinglimin wrote:

> > Replaced a call to malloc() and its respe= ctive call to free() with g_malloc() and g_free().

> >

> > Signed-off-by: dinglimin <dinglimin@cmss.chinamobil= e.com>

> >

> > V1 -> V2:if cpu_m= emory_rw_debug failed, still need to set p=3DNULL

> > ---

> >=C2=A0=C2=A0 semihosting/uaccess.c | 4 ++--

> >=C2=A0=C2=A0 1 file chan= ged, 2 insertions(+), 2 deletions(-)

> >

>= ; > diff --git a/semihosting/uaccess.c b/semihosting/uaccess.c

> > index 8018828069..2ac754= cdb6 100644

> > ---= a/semihosting/uaccess.c

= > > +++ b/semihosting/uaccess.c

> > @@ -14,10 +14,10 @@

<= span lang=3DEN-US>> >=C2=A0=C2=A0 void *softmmu_lock_user(CPUArchStat= e *env, target_ulong addr,

> >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 target_ulong len, bool copy)

> >=C2=A0=C2=A0 {

> > -=C2=A0=C2=A0=C2=A0 void *p =3D malloc(len)= ;

> > +=C2=A0=C2=A0= =C2=A0 void *p =3D g_malloc(len);

> >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (p && cop= y) {

> >=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0if (cpu_memory_rw_debug= (env_cpu(env), addr, p, len, 0)) {

> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 free(p);

>= ; > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 = g_free(p);

> >=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 p =3D NULL;

> &= gt;=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }

> >=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 }

>&= nbsp;

> Ok, that= was the obvious part.=C2=A0 Now a next one, also obvious.

> 

> You changed lock_user to use g_malloc(= ), but unlock_user

> s= till uses free() instead of g_free().=C2=A0 At the very least

> the other one needs to be changed= too.=C2=A0 And I'd say the callers

> should be analyzed to ensure they don't free() the result

> (they should not, thi= nk it is a bug if they do).

 

We c= an be pretty sure the callers don't free() the returned

value, because the calling code is also use= d 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*.<= /p>

 

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 "w= e expect this might fail" (which we want

here, because the guest is passing us the length of me= mory

to try to allocate) = is g_try_malloc().

&= nbsp;

thanks=

-- PMM

 

= --_3DD5A187-4286-4919-9294-C9508DF89758_--