| Message ID | 1412996048-6384-1-git-send-email-arei.gonglei@huawei.com |
|---|---|
| State | New |
| Headers | show |
On 2014/10/11 10:54, arei.gonglei@huawei.com wrote: > From: Gonglei <arei.gonglei@huawei.com> > > The caller of qemu_vfree() maybe not check whether parameter > ptr pointer is NULL or not, such as vpc_open(). > Using g_free() is more safe. > It seems that free(NULL) is harmless. From section 7.20.3.2/2 of the C99 standard: The free function causes the space pointed to by ptr to be deallocated, that is, made available for further allocation. If ptr is a null pointer, no action occurs. > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > --- > util/oslib-posix.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > index 016a047..ca435d0 100644 > --- a/util/oslib-posix.c > +++ b/util/oslib-posix.c > @@ -153,7 +153,7 @@ void *qemu_anon_ram_alloc(size_t size) > void qemu_vfree(void *ptr) > { > trace_qemu_vfree(ptr); > - free(ptr); > + g_free(ptr); > } > > void qemu_anon_ram_free(void *ptr, size_t size) >
On 2014/10/11 11:10, Zhanghailiang wrote: > On 2014/10/11 10:54, arei.gonglei@huawei.com wrote: >> From: Gonglei <arei.gonglei@huawei.com> >> >> The caller of qemu_vfree() maybe not check whether parameter >> ptr pointer is NULL or not, such as vpc_open(). >> Using g_free() is more safe. >> > > It seems that free(NULL) is harmless. > Actually, I had noted that C standard says it is a no-operation. But that doesn't mean that every C-library handles it like that. Some people saw crashes for free(NULL), so it's best to avoid calling the free in the first place (caller) or using g_free() in qemu_vfree(). Best regards, -Gonglei > From section 7.20.3.2/2 of the C99 standard: > The free function causes the space pointed to by ptr to be deallocated, that is, > made available for further allocation. If ptr is a null pointer, no action occurs. > >> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >> --- >> util/oslib-posix.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/util/oslib-posix.c b/util/oslib-posix.c >> index 016a047..ca435d0 100644 >> --- a/util/oslib-posix.c >> +++ b/util/oslib-posix.c >> @@ -153,7 +153,7 @@ void *qemu_anon_ram_alloc(size_t size) >> void qemu_vfree(void *ptr) >> { >> trace_qemu_vfree(ptr); >> - free(ptr); >> + g_free(ptr); >> } >> >> void qemu_anon_ram_free(void *ptr, size_t size) >> > >
On 10/10/2014 08:54 PM, arei.gonglei@huawei.com wrote: > From: Gonglei <arei.gonglei@huawei.com> > > The caller of qemu_vfree() maybe not check whether parameter > ptr pointer is NULL or not, such as vpc_open(). > Using g_free() is more safe. NACK. g_free is only safe for pointers allocated by g_malloc. qemu_vfree is for use on pointers allocated by qemu_try_memalign and friends (matching the name valloc which is an older spelling of posix_memalign), which are NOT allocated by g_malloc. Furthermore, free(NULL) is safe.
On 10/10/2014 09:21 PM, Gonglei wrote: > > Actually, I had noted that C standard says it is a no-operation. > But that doesn't mean that every C-library handles it like that. EVERY libc that is C89 compliant handles it like that. The last platform that failed on free(NULL) was SunOS 4, which is such museum-ware it's not funny. There is no need to cater to platforms from 25 years ago. > Some people saw crashes for free(NULL), so it's best to avoid > calling the free in the first place (caller) or using g_free() in qemu_vfree(). Absolutely not. g_free is unsafe to use except for pointers from g_malloc, which is NOT the case that qemu_vfree is used on.
On 2014/10/11 11:26, Eric Blake wrote: > On 10/10/2014 09:21 PM, Gonglei wrote: > >> >> Actually, I had noted that C standard says it is a no-operation. >> But that doesn't mean that every C-library handles it like that. > > EVERY libc that is C89 compliant handles it like that. The last > platform that failed on free(NULL) was SunOS 4, which is such > museum-ware it's not funny. There is no need to cater to platforms from > 25 years ago. > OK.Thanks for explanation! >> Some people saw crashes for free(NULL), so it's best to avoid >> calling the free in the first place (caller) or using g_free() in qemu_vfree(). > > Absolutely not. g_free is unsafe to use except for pointers from > g_malloc, which is NOT the case that qemu_vfree is used on. > Got it, thanks :) But why some callers make a check, but some other callers don't do this check? Can I consider those check is superfluous? -Best regards, -Gonglei
On 10/10/2014 09:32 PM, Gonglei wrote: >>> Actually, I had noted that C standard says it is a no-operation. >>> But that doesn't mean that every C-library handles it like that. >> >> EVERY libc that is C89 compliant handles it like that. The last >> platform that failed on free(NULL) was SunOS 4, which is such >> museum-ware it's not funny. There is no need to cater to platforms from >> 25 years ago. > > But why some callers make a check, > but some other callers don't do this check? Because some people haven't learned that free(NULL) is safe yet. You're welcome to simplify code as you touch it. > Can I consider those check is superfluous? Yes. Checking for NULL before calling free() or g_free() is wasted effort.
On 2014/10/11 11:44, Eric Blake wrote: > On 10/10/2014 09:32 PM, Gonglei wrote: > >>>> Actually, I had noted that C standard says it is a no-operation. >>>> But that doesn't mean that every C-library handles it like that. >>> >>> EVERY libc that is C89 compliant handles it like that. The last >>> platform that failed on free(NULL) was SunOS 4, which is such >>> museum-ware it's not funny. There is no need to cater to platforms from >>> 25 years ago. > >> >> But why some callers make a check, >> but some other callers don't do this check? > > Because some people haven't learned that free(NULL) is safe yet. You're > welcome to simplify code as you touch it. > OK, I will. Thanks again :) >> Can I consider those check is superfluous? > > Yes. Checking for NULL before calling free() or g_free() is wasted effort. > Best regards, -Gonglei
Am 11.10.2014 um 05:23 hat Eric Blake geschrieben: > On 10/10/2014 08:54 PM, arei.gonglei@huawei.com wrote: > > From: Gonglei <arei.gonglei@huawei.com> > > > > The caller of qemu_vfree() maybe not check whether parameter > > ptr pointer is NULL or not, such as vpc_open(). > > Using g_free() is more safe. > > NACK. g_free is only safe for pointers allocated by g_malloc. > qemu_vfree is for use on pointers allocated by qemu_try_memalign and > friends (matching the name valloc which is an older spelling of > posix_memalign), which are NOT allocated by g_malloc. Furthermore, > free(NULL) is safe. I second that. Strong NACK. Kevin
diff --git a/util/oslib-posix.c b/util/oslib-posix.c index 016a047..ca435d0 100644 --- a/util/oslib-posix.c +++ b/util/oslib-posix.c @@ -153,7 +153,7 @@ void *qemu_anon_ram_alloc(size_t size) void qemu_vfree(void *ptr) { trace_qemu_vfree(ptr); - free(ptr); + g_free(ptr); } void qemu_anon_ram_free(void *ptr, size_t size)