Re: [libvirt] [PATCH]: Fix VIR_ALLOC_N for 0 byte arrays
On Thu, Jun 19, 2008 at 05:51:07AM -0400, Daniel Veillard wrote: > On Thu, Jun 19, 2008 at 10:49:59AM +0200, Chris Lalancette wrote: > > Hello, > > For 0.4.3, danpb's new memory management scheme went into libvirt. > > This is > > fine, except that is subtly alters the semantics of malloc(), calloc(), and > > realloc(). In particular, if you say: > > > > foo = malloc(0); > > > > glibc will happily return a non-NULL pointer to you. However, with the new > > memory management stuff, if you say: > > > > foo = VIR_ALLOC(0); > > > > you will actually get a NULL pointer back. Personally, I think this is a > > dangerous deviation from malloc() semantics that everyone is used to, and is > > indeed causing problems with the remote driver. The short of it is that the > > remote driver allocates memory on behalf of the remote side using > > VIR_ALLOC_N, > > and this call is returning NULL so that the NULL checks elsewhere in the > > code > > fire and return failure. > > > > The attached patch fixes this situation by removing the 0 checks from the > > memory > > allocation paths, and just lets them fall through to the normal malloc(), > > calloc(), or realloc() routines, restoring old semantics. > > > > Signed-off-by: Chris Lalancette <[EMAIL PROTECTED]> > > Agreed, it's a problem, +1, but > since Dan explicitely made the == 0 test to return NULL he probably > had a purpose about this (I suspect detecting 0 sized memory allocations). No, this was just a stupid bug on my part. This patch is fine. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH]: Fix VIR_ALLOC_N for 0 byte arrays
On Thu, Jun 19, 2008 at 10:49:59AM +0200, Chris Lalancette wrote: > Hello, > For 0.4.3, danpb's new memory management scheme went into libvirt. This > is > fine, except that is subtly alters the semantics of malloc(), calloc(), and > realloc(). In particular, if you say: > > foo = malloc(0); > > glibc will happily return a non-NULL pointer to you. However, with the new > memory management stuff, if you say: > > foo = VIR_ALLOC(0); > > you will actually get a NULL pointer back. Personally, I think this is a > dangerous deviation from malloc() semantics that everyone is used to, and is > indeed causing problems with the remote driver. The short of it is that the > remote driver allocates memory on behalf of the remote side using VIR_ALLOC_N, > and this call is returning NULL so that the NULL checks elsewhere in the code > fire and return failure. > > The attached patch fixes this situation by removing the 0 checks from the > memory > allocation paths, and just lets them fall through to the normal malloc(), > calloc(), or realloc() routines, restoring old semantics. > > Signed-off-by: Chris Lalancette <[EMAIL PROTECTED]> Agreed, it's a problem, +1, but since Dan explicitely made the == 0 test to return NULL he probably had a purpose about this (I suspect detecting 0 sized memory allocations). So i would not apply the patch before he had a chance to comment on it, but yes I personally thing we should reverse that bit. A zero lenght allocation is legal, and may be later extended with realloc. This can lead to vastly simplified code (to the expense of a seemingly useless initial allocation). Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ [EMAIL PROTECTED] | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH]: Fix VIR_ALLOC_N for 0 byte arrays
Chris Lalancette <[EMAIL PROTECTED]> wrote: > For 0.4.3, danpb's new memory management scheme went into libvirt. This > is > fine, except that is subtly alters the semantics of malloc(), calloc(), and > realloc(). In particular, if you say: > > foo = malloc(0); > > glibc will happily return a non-NULL pointer to you. However, with the new > memory management stuff, if you say: > > foo = VIR_ALLOC(0); > > you will actually get a NULL pointer back. Personally, I think this is a > dangerous deviation from malloc() semantics that everyone is used to, and is > indeed causing problems with the remote driver. The short of it is that the > remote driver allocates memory on behalf of the remote side using VIR_ALLOC_N, > and this call is returning NULL so that the NULL checks elsewhere in the code > fire and return failure. > > The attached patch fixes this situation by removing the 0 checks from the > memory > allocation paths, and just lets them fall through to the normal malloc(), > calloc(), or realloc() routines, restoring old semantics. Wow. That's a nasty one, violating such assumptions. Good catch. In case anyone reading along wonders about the portability of libvirt's assumption that those functions return non-NULL for N=0, it's fine because gnulib wrappers protect us from libc functions with the undesirable semantics. ACK. -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH]: Fix VIR_ALLOC_N for 0 byte arrays
Hello,
For 0.4.3, danpb's new memory management scheme went into libvirt. This is
fine, except that is subtly alters the semantics of malloc(), calloc(), and
realloc(). In particular, if you say:
foo = malloc(0);
glibc will happily return a non-NULL pointer to you. However, with the new
memory management stuff, if you say:
foo = VIR_ALLOC(0);
you will actually get a NULL pointer back. Personally, I think this is a
dangerous deviation from malloc() semantics that everyone is used to, and is
indeed causing problems with the remote driver. The short of it is that the
remote driver allocates memory on behalf of the remote side using VIR_ALLOC_N,
and this call is returning NULL so that the NULL checks elsewhere in the code
fire and return failure.
The attached patch fixes this situation by removing the 0 checks from the memory
allocation paths, and just lets them fall through to the normal malloc(),
calloc(), or realloc() routines, restoring old semantics.
Signed-off-by: Chris Lalancette <[EMAIL PROTECTED]>
Index: src/memory.c
===
RCS file: /data/cvs/libvirt/src/memory.c,v
retrieving revision 1.7
diff -u -r1.7 memory.c
--- a/src/memory.c 6 Jun 2008 11:09:57 - 1.7
+++ b/src/memory.c 19 Jun 2008 08:44:01 -
@@ -97,11 +97,6 @@
}
#endif
-if (size == 0) {
-*(void **)ptrptr = NULL;
-return 0;
-}
-
*(void **)ptrptr = calloc(1, size);
if (*(void **)ptrptr == NULL)
return -1;
@@ -130,11 +125,6 @@
}
#endif
-if (size == 0 || count == 0) {
-*(void **)ptrptr = NULL;
-return 0;
-}
-
*(void**)ptrptr = calloc(count, size);
if (*(void**)ptrptr == NULL)
return -1;
@@ -163,11 +153,6 @@
return -1;
#endif
-if (size == 0 || count == 0) {
-free(*(void **)ptrptr);
-*(void **)ptrptr = NULL;
-return 0;
-}
if (xalloc_oversized(count, size)) {
errno = ENOMEM;
return -1;
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
