[libvirt] [ruby-libvirt] Don't free more entries than we retrieved

The vir*List* functions return the number of fetched entries. We mustn't free more, otherwise we'll crash like #0 0xb779d424 in __kernel_vsyscall () #1 0xb733981f in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 #2 0xb733ccd3 in __GI_abort () at abort.c:90 #3 0xb7376275 in __libc_message (do_abort=do_abort@entry=2, fmt=fmt@entry=0xb74767d0 "*** Error in `%s': %s: 0x%s ***\n") at ../sysdeps/unix/sysv/linux/libc_fatal.c:199 #4 0xb7380e52 in malloc_printerr (action=<optimized out>, str=<optimized out>, ptr=0xb7087000) at malloc.c:4923 #5 0xb7381b90 in _int_free (av=0xb74b7440 <main_arena>, p=0xb7086ff8, have_lock=0) at malloc.c:3779 #6 0xb75c059f in ruby_xfree () from /usr/lib/libruby-1.9.1.so.1.9 #7 0xb7076448 in ruby_libvirt_generate_list () from /usr/lib/ruby/vendor_ruby/1.9.1/i486-linux/_libvirt.so ... since we're trying to free random addresses. --- ext/libvirt/connect.c | 4 ++-- ext/libvirt/domain.c | 2 +- ext/libvirt/nodedevice.c | 2 +- ext/libvirt/storage.c | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/ext/libvirt/connect.c b/ext/libvirt/connect.c index 36cac20..1af64a1 100644 --- a/ext/libvirt/connect.c +++ b/ext/libvirt/connect.c @@ -67,7 +67,7 @@ names = alloca(sizeof(char *) * num); \ r = virConnectList##objs(ruby_libvirt_connect_get(c), names, num); \ ruby_libvirt_raise_error_if(r < 0, e_RetrieveError, "virConnectList" # objs, ruby_libvirt_connect_get(c)); \ - return ruby_libvirt_generate_list(num, names); \ + return ruby_libvirt_generate_list(r < 0 ? 0 : r, names); \ } while(0) static VALUE c_connect; @@ -1508,7 +1508,7 @@ static VALUE libvirt_connect_list_nodedevices(int argc, VALUE *argv, VALUE c) ruby_libvirt_raise_error_if(r < 0, e_RetrieveError, "virNodeListDevices", ruby_libvirt_connect_get(c)); - return ruby_libvirt_generate_list(num, names); + return ruby_libvirt_generate_list(r < 0 ? 0 : r, names); } /* diff --git a/ext/libvirt/domain.c b/ext/libvirt/domain.c index ddce2d8..bc46753 100644 --- a/ext/libvirt/domain.c +++ b/ext/libvirt/domain.c @@ -1532,7 +1532,7 @@ static VALUE libvirt_domain_list_snapshots(int argc, VALUE *argv, VALUE d) "virDomainSnapshotListNames", ruby_libvirt_connect_get(d)); - return ruby_libvirt_generate_list(num, names); + return ruby_libvirt_generate_list(r < 0 ? 0 : r, names); } /* diff --git a/ext/libvirt/nodedevice.c b/ext/libvirt/nodedevice.c index 98b3715..041cda2 100644 --- a/ext/libvirt/nodedevice.c +++ b/ext/libvirt/nodedevice.c @@ -124,7 +124,7 @@ static VALUE libvirt_nodedevice_list_caps(VALUE c) "virNodeDeviceListCaps", ruby_libvirt_connect_get(c)); - return ruby_libvirt_generate_list(num, names); + return ruby_libvirt_generate_list(r < 0 ? 0 : r, names); } /* diff --git a/ext/libvirt/storage.c b/ext/libvirt/storage.c index 4b96d2e..008410a 100644 --- a/ext/libvirt/storage.c +++ b/ext/libvirt/storage.c @@ -340,7 +340,7 @@ static VALUE libvirt_storage_pool_list_volumes(VALUE p) "virStoragePoolListVolumes", ruby_libvirt_connect_get(p)); - return ruby_libvirt_generate_list(num, names); + return ruby_libvirt_generate_list(r < 0 ? 0 : r, names); } /* -- 1.8.5.2

On Tue, Jan 7, 2014 at 3:24 PM, Guido Günther <agx@sigxcpu.org> wrote:
The vir*List* functions return the number of fetched entries. We mustn't free more, otherwise we'll crash like
#0 0xb779d424 in __kernel_vsyscall () #1 0xb733981f in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 #2 0xb733ccd3 in __GI_abort () at abort.c:90 #3 0xb7376275 in __libc_message (do_abort=do_abort@entry=2, fmt=fmt@entry=0xb74767d0 "*** Error in `%s': %s: 0x%s ***\n") at ../sysdeps/unix/sysv/linux/libc_fatal.c:199 #4 0xb7380e52 in malloc_printerr (action=<optimized out>, str=<optimized out>, ptr=0xb7087000) at malloc.c:4923 #5 0xb7381b90 in _int_free (av=0xb74b7440 <main_arena>, p=0xb7086ff8, have_lock=0) at malloc.c:3779 #6 0xb75c059f in ruby_xfree () from /usr/lib/libruby-1.9.1.so.1.9 #7 0xb7076448 in ruby_libvirt_generate_list () from /usr/lib/ruby/vendor_ruby/1.9.1/i486-linux/_libvirt.so ...
since we're trying to free random addresses.
Hm, this is pretty weird. At all ruby_libvirt_generate_list() call sites, there is a line of code that precedes it that checks for r < 0. If r is less than 0, then we generate an exception, which means that the code that follows should never be called, and thus we should never need this patch. Can you explain a bit more why this patch helps, and/or give me a test case that will cause it to fail? Thanks, Chris

On Tue, Jan 07, 2014 at 03:38:48PM -0500, Chris Lalancette wrote:
On Tue, Jan 7, 2014 at 3:24 PM, Guido Günther <agx@sigxcpu.org> wrote:
The vir*List* functions return the number of fetched entries. We mustn't free more, otherwise we'll crash like
#0 0xb779d424 in __kernel_vsyscall () #1 0xb733981f in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 #2 0xb733ccd3 in __GI_abort () at abort.c:90 #3 0xb7376275 in __libc_message (do_abort=do_abort@entry=2, fmt=fmt@entry=0xb74767d0 "*** Error in `%s': %s: 0x%s ***\n") at ../sysdeps/unix/sysv/linux/libc_fatal.c:199 #4 0xb7380e52 in malloc_printerr (action=<optimized out>, str=<optimized out>, ptr=0xb7087000) at malloc.c:4923 #5 0xb7381b90 in _int_free (av=0xb74b7440 <main_arena>, p=0xb7086ff8, have_lock=0) at malloc.c:3779 #6 0xb75c059f in ruby_xfree () from /usr/lib/libruby-1.9.1.so.1.9 #7 0xb7076448 in ruby_libvirt_generate_list () from /usr/lib/ruby/vendor_ruby/1.9.1/i486-linux/_libvirt.so ...
since we're trying to free random addresses.
Hm, this is pretty weird. At all ruby_libvirt_generate_list() call sites, there is a line of code that precedes it that checks for r < 0. If r is less than 0, then we generate an exception, which means that the code that follows should never be called, and thus we should never need this patch.
The check for < 0 might be superfluos but freeing num entries instead of only r ones looks wrong to me since r is the number of allocated names, not num. I'm happy to send a patch dropping the < 0 part.
Can you explain a bit more why this patch helps, and/or give me a test case that will cause it to fail?
I've been running test_nodedevice as non root which triggered this. Cheers, -- Guido
Thanks, Chris

On Tue, Jan 7, 2014 at 4:04 PM, Guido Günther <agx@sigxcpu.org> wrote:
The check for < 0 might be superfluos but freeing num entries instead of only r ones looks wrong to me since r is the number of allocated names, not num. I'm happy to send a patch dropping the < 0 part.
Oh, bah humbug. Yeah, you are definitely right about that. Please do send a patch with just the num -> r bit. That is definitely a bug. Thanks, Chris

The vir*List* functions return the number of fetched entries. We mustn't free more, otherwise we'll crash like #0 0xb779d424 in __kernel_vsyscall () #1 0xb733981f in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 #2 0xb733ccd3 in __GI_abort () at abort.c:90 #3 0xb7376275 in __libc_message (do_abort=do_abort@entry=2, fmt=fmt@entry=0xb74767d0 "*** Error in `%s': %s: 0x%s ***\n") at ../sysdeps/unix/sysv/linux/libc_fatal.c:199 #4 0xb7380e52 in malloc_printerr (action=<optimized out>, str=<optimized out>, ptr=0xb7087000) at malloc.c:4923 #5 0xb7381b90 in _int_free (av=0xb74b7440 <main_arena>, p=0xb7086ff8, have_lock=0) at malloc.c:3779 #6 0xb75c059f in ruby_xfree () from /usr/lib/libruby-1.9.1.so.1.9 #7 0xb7076448 in ruby_libvirt_generate_list () from /usr/lib/ruby/vendor_ruby/1.9.1/i486-linux/_libvirt.so ... since we're trying to free random addresses. --- ext/libvirt/connect.c | 4 ++-- ext/libvirt/domain.c | 2 +- ext/libvirt/nodedevice.c | 2 +- ext/libvirt/storage.c | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/ext/libvirt/connect.c b/ext/libvirt/connect.c index 36cac20..cef4de1 100644 --- a/ext/libvirt/connect.c +++ b/ext/libvirt/connect.c @@ -67,7 +67,7 @@ names = alloca(sizeof(char *) * num); \ r = virConnectList##objs(ruby_libvirt_connect_get(c), names, num); \ ruby_libvirt_raise_error_if(r < 0, e_RetrieveError, "virConnectList" # objs, ruby_libvirt_connect_get(c)); \ - return ruby_libvirt_generate_list(num, names); \ + return ruby_libvirt_generate_list(r, names); \ } while(0) static VALUE c_connect; @@ -1508,7 +1508,7 @@ static VALUE libvirt_connect_list_nodedevices(int argc, VALUE *argv, VALUE c) ruby_libvirt_raise_error_if(r < 0, e_RetrieveError, "virNodeListDevices", ruby_libvirt_connect_get(c)); - return ruby_libvirt_generate_list(num, names); + return ruby_libvirt_generate_list(r, names); } /* diff --git a/ext/libvirt/domain.c b/ext/libvirt/domain.c index ddce2d8..8ac7d99 100644 --- a/ext/libvirt/domain.c +++ b/ext/libvirt/domain.c @@ -1532,7 +1532,7 @@ static VALUE libvirt_domain_list_snapshots(int argc, VALUE *argv, VALUE d) "virDomainSnapshotListNames", ruby_libvirt_connect_get(d)); - return ruby_libvirt_generate_list(num, names); + return ruby_libvirt_generate_list(r, names); } /* diff --git a/ext/libvirt/nodedevice.c b/ext/libvirt/nodedevice.c index 98b3715..f0cbb4b 100644 --- a/ext/libvirt/nodedevice.c +++ b/ext/libvirt/nodedevice.c @@ -124,7 +124,7 @@ static VALUE libvirt_nodedevice_list_caps(VALUE c) "virNodeDeviceListCaps", ruby_libvirt_connect_get(c)); - return ruby_libvirt_generate_list(num, names); + return ruby_libvirt_generate_list(r, names); } /* diff --git a/ext/libvirt/storage.c b/ext/libvirt/storage.c index 4b96d2e..a01edbc 100644 --- a/ext/libvirt/storage.c +++ b/ext/libvirt/storage.c @@ -340,7 +340,7 @@ static VALUE libvirt_storage_pool_list_volumes(VALUE p) "virStoragePoolListVolumes", ruby_libvirt_connect_get(p)); - return ruby_libvirt_generate_list(num, names); + return ruby_libvirt_generate_list(r, names); } /* -- 1.8.5.2

On Tue, Jan 7, 2014 at 4:54 PM, Guido Günther <agx@sigxcpu.org> wrote:
The vir*List* functions return the number of fetched entries. We mustn't free more, otherwise we'll crash like
#0 0xb779d424 in __kernel_vsyscall () #1 0xb733981f in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 #2 0xb733ccd3 in __GI_abort () at abort.c:90 #3 0xb7376275 in __libc_message (do_abort=do_abort@entry=2, fmt=fmt@entry=0xb74767d0 "*** Error in `%s': %s: 0x%s ***\n") at ../sysdeps/unix/sysv/linux/libc_fatal.c:199 #4 0xb7380e52 in malloc_printerr (action=<optimized out>, str=<optimized out>, ptr=0xb7087000) at malloc.c:4923 #5 0xb7381b90 in _int_free (av=0xb74b7440 <main_arena>, p=0xb7086ff8, have_lock=0) at malloc.c:3779 #6 0xb75c059f in ruby_xfree () from /usr/lib/libruby-1.9.1.so.1.9 #7 0xb7076448 in ruby_libvirt_generate_list () from /usr/lib/ruby/vendor_ruby/1.9.1/i486-linux/_libvirt.so ...
since we're trying to free random addresses.
Thanks, I've applied this patch now. This is probably worthy of another release, since it could be quite a bad bug. That being said, are you actively doing additional testing? If so, I'll wait a bit longer to see if you come up with anything else. Chris

On Tue, Jan 07, 2014 at 08:55:04PM -0500, Chris Lalancette wrote:
On Tue, Jan 7, 2014 at 4:54 PM, Guido Günther <agx@sigxcpu.org> wrote:
The vir*List* functions return the number of fetched entries. We mustn't free more, otherwise we'll crash like
#0 0xb779d424 in __kernel_vsyscall () #1 0xb733981f in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 #2 0xb733ccd3 in __GI_abort () at abort.c:90 #3 0xb7376275 in __libc_message (do_abort=do_abort@entry=2, fmt=fmt@entry=0xb74767d0 "*** Error in `%s': %s: 0x%s ***\n") at ../sysdeps/unix/sysv/linux/libc_fatal.c:199 #4 0xb7380e52 in malloc_printerr (action=<optimized out>, str=<optimized out>, ptr=0xb7087000) at malloc.c:4923 #5 0xb7381b90 in _int_free (av=0xb74b7440 <main_arena>, p=0xb7086ff8, have_lock=0) at malloc.c:3779 #6 0xb75c059f in ruby_xfree () from /usr/lib/libruby-1.9.1.so.1.9 #7 0xb7076448 in ruby_libvirt_generate_list () from /usr/lib/ruby/vendor_ruby/1.9.1/i486-linux/_libvirt.so ...
since we're trying to free random addresses.
Thanks, I've applied this patch now. This is probably worthy of
Thanks!
another release, since it could be quite a bad bug. That being said, are you actively doing additional testing? If so, I'll wait a bit longer to see if you come up with anything else.
Unfortunately I won't have time to work more on this during the next days. I plan to run more time during Debian package builds in the future but this will have to wait for a bit more free time. Cheers, -- Guido
participants (2)
-
Chris Lalancette
-
Guido Günther