[libvirt] [PATCH] ruby-libvirt: Don't crash in leases_wrap() by passing NULLs to rb_str_new2()
Not all lease values are mandatory, and when they aren't supplied by the libvirt driver they get set to NULL. That makes rb_str_new2() bail out. Signed-off-by: Dan Williams <dcbw@redhat.com> --- For example using the qemu driver we don't get 'iaid', and that makes ruby unhappy: [{"iface"=>"virbr1", "expirytime"=>1452189569, "type"=>0, "mac"=>"52:54:00:05:2b:6f", "ipaddr"=>"192.168.121.49", "prefix"=>24, "hostname"=>"openshiftdev", "clientid"=>"ff:00:05:2b:6f:00:01:00:01:1e:21:55:ed:52:54:00:05:2b:6f"}] ext/libvirt/network.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/ext/libvirt/network.c b/ext/libvirt/network.c index 7c77d73..c250d7d 100644 --- a/ext/libvirt/network.c +++ b/ext/libvirt/network.c @@ -269,14 +269,20 @@ static VALUE leases_wrap(VALUE arg) rb_hash_aset(hash, rb_str_new2("expirytime"), LL2NUM(lease->expirytime)); rb_hash_aset(hash, rb_str_new2("type"), INT2NUM(lease->type)); - rb_hash_aset(hash, rb_str_new2("mac"), rb_str_new2(lease->mac)); - rb_hash_aset(hash, rb_str_new2("iaid"), rb_str_new2(lease->iaid)); + if (lease->mac) + rb_hash_aset(hash, rb_str_new2("mac"), rb_str_new2(lease->mac)); + if (lease->iaid) + rb_hash_aset(hash, rb_str_new2("iaid"), rb_str_new2(lease->iaid)); rb_hash_aset(hash, rb_str_new2("ipaddr"), rb_str_new2(lease->ipaddr)); rb_hash_aset(hash, rb_str_new2("prefix"), UINT2NUM(lease->prefix)); - rb_hash_aset(hash, rb_str_new2("hostname"), - rb_str_new2(lease->hostname)); - rb_hash_aset(hash, rb_str_new2("clientid"), - rb_str_new2(lease->clientid)); + if (lease->hostname) { + rb_hash_aset(hash, rb_str_new2("hostname"), + rb_str_new2(lease->hostname)); + } + if (lease->clientid) { + rb_hash_aset(hash, rb_str_new2("clientid"), + rb_str_new2(lease->clientid)); + } rb_ary_store(result, i, hash); } -- 2.4.3
On Thu, 2016-01-07 at 11:12 -0600, Dan Williams wrote:
Not all lease values are mandatory, and when they aren't supplied by the libvirt driver they get set to NULL. That makes rb_str_new2() bail out.
Ping? Does this patch look OK or is there anything else I need to do with it? Is the submission procure for ruby-libvirt different than normal libvirt? Thanks! Dan
Signed-off-by: Dan Williams <dcbw@redhat.com> --- For example using the qemu driver we don't get 'iaid', and that makes ruby unhappy:
[{"iface"=>"virbr1", "expirytime"=>1452189569, "type"=>0, "mac"=>"52:54:00:05:2b:6f", "ipaddr"=>"192.168.121.49", "prefix"=>24, "hostname"=>"openshiftdev", "clientid"=>"ff:00:05:2b:6f:00:01:00:01:1e:21:55:ed:52:54:00:05:2b:6f "}]
ext/libvirt/network.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/ext/libvirt/network.c b/ext/libvirt/network.c index 7c77d73..c250d7d 100644 --- a/ext/libvirt/network.c +++ b/ext/libvirt/network.c @@ -269,14 +269,20 @@ static VALUE leases_wrap(VALUE arg) rb_hash_aset(hash, rb_str_new2("expirytime"), LL2NUM(lease->expirytime)); rb_hash_aset(hash, rb_str_new2("type"), INT2NUM(lease ->type)); - rb_hash_aset(hash, rb_str_new2("mac"), rb_str_new2(lease ->mac)); - rb_hash_aset(hash, rb_str_new2("iaid"), rb_str_new2(lease ->iaid)); + if (lease->mac) + rb_hash_aset(hash, rb_str_new2("mac"), rb_str_new2(lease ->mac)); + if (lease->iaid) + rb_hash_aset(hash, rb_str_new2("iaid"), rb_str_new2(lease->iaid)); rb_hash_aset(hash, rb_str_new2("ipaddr"), rb_str_new2(lease ->ipaddr)); rb_hash_aset(hash, rb_str_new2("prefix"), UINT2NUM(lease ->prefix)); - rb_hash_aset(hash, rb_str_new2("hostname"), - rb_str_new2(lease->hostname)); - rb_hash_aset(hash, rb_str_new2("clientid"), - rb_str_new2(lease->clientid)); + if (lease->hostname) { + rb_hash_aset(hash, rb_str_new2("hostname"), + rb_str_new2(lease->hostname)); + } + if (lease->clientid) { + rb_hash_aset(hash, rb_str_new2("clientid"), + rb_str_new2(lease->clientid)); + }
rb_ary_store(result, i, hash); }
On 01/14/2016 11:01 AM, Dan Williams wrote:
On Thu, 2016-01-07 at 11:12 -0600, Dan Williams wrote:
Not all lease values are mandatory, and when they aren't supplied by the libvirt driver they get set to NULL. That makes rb_str_new2() bail out. Ping? Does this patch look OK or is there anything else I need to do with it? Is the submission procure for ruby-libvirt different than normal libvirt?
As far as I can see, posting to libvir-list is the correct thing for ruby-libvirt patches, I think it's just that very few people use it, so most of us don't feel comfortable ACKing anything for it. It looks like Chris Lalancette is the maintainer and has been the author of nearly every patch in the last couple years, so I'm Cc'ing him to be sure it see it.
Thanks! Dan
Signed-off-by: Dan Williams <dcbw@redhat.com> --- For example using the qemu driver we don't get 'iaid', and that makes ruby unhappy:
[{"iface"=>"virbr1", "expirytime"=>1452189569, "type"=>0, "mac"=>"52:54:00:05:2b:6f", "ipaddr"=>"192.168.121.49", "prefix"=>24, "hostname"=>"openshiftdev", "clientid"=>"ff:00:05:2b:6f:00:01:00:01:1e:21:55:ed:52:54:00:05:2b:6f "}]
ext/libvirt/network.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/ext/libvirt/network.c b/ext/libvirt/network.c index 7c77d73..c250d7d 100644 --- a/ext/libvirt/network.c +++ b/ext/libvirt/network.c @@ -269,14 +269,20 @@ static VALUE leases_wrap(VALUE arg) rb_hash_aset(hash, rb_str_new2("expirytime"), LL2NUM(lease->expirytime)); rb_hash_aset(hash, rb_str_new2("type"), INT2NUM(lease ->type)); - rb_hash_aset(hash, rb_str_new2("mac"), rb_str_new2(lease ->mac)); - rb_hash_aset(hash, rb_str_new2("iaid"), rb_str_new2(lease ->iaid)); + if (lease->mac) + rb_hash_aset(hash, rb_str_new2("mac"), rb_str_new2(lease ->mac)); + if (lease->iaid) + rb_hash_aset(hash, rb_str_new2("iaid"), rb_str_new2(lease->iaid)); rb_hash_aset(hash, rb_str_new2("ipaddr"), rb_str_new2(lease ->ipaddr)); rb_hash_aset(hash, rb_str_new2("prefix"), UINT2NUM(lease ->prefix)); - rb_hash_aset(hash, rb_str_new2("hostname"), - rb_str_new2(lease->hostname)); - rb_hash_aset(hash, rb_str_new2("clientid"), - rb_str_new2(lease->clientid)); + if (lease->hostname) { + rb_hash_aset(hash, rb_str_new2("hostname"), + rb_str_new2(lease->hostname)); + } + if (lease->clientid) { + rb_hash_aset(hash, rb_str_new2("clientid"), + rb_str_new2(lease->clientid)); + }
rb_ary_store(result, i, hash); } -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 2016-01-14 at 13:19 -0500, Laine Stump wrote:
On 01/14/2016 11:01 AM, Dan Williams wrote:
On Thu, 2016-01-07 at 11:12 -0600, Dan Williams wrote:
Not all lease values are mandatory, and when they aren't supplied by the libvirt driver they get set to NULL. That makes rb_str_new2() bail out. Ping? Does this patch look OK or is there anything else I need to do with it? Is the submission procure for ruby-libvirt different than normal libvirt?
As far as I can see, posting to libvir-list is the correct thing for ruby-libvirt patches, I think it's just that very few people use it, so most of us don't feel comfortable ACKing anything for it.
Thanks! I'd bet a lot more people use it than you think, since it's a dependency of vagrant-libvirt by way of fog. So you can't stand up a vagrant machine using libvirt without it... Dan
It looks like Chris Lalancette is the maintainer and has been the author of nearly every patch in the last couple years, so I'm Cc'ing him to be sure it see it.
Thanks! Dan
Signed-off-by: Dan Williams <dcbw@redhat.com> --- For example using the qemu driver we don't get 'iaid', and that makes ruby unhappy:
[{"iface"=>"virbr1", "expirytime"=>1452189569, "type"=>0, "mac"=>"52:54:00:05:2b:6f", "ipaddr"=>"192.168.121.49", "prefix"=>24, "hostname"=>"openshiftdev", "clientid"=>"ff:00:05:2b:6f:00:01:00:01:1e:21:55:ed:52:54:00:05:2 b:6f "}]
ext/libvirt/network.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/ext/libvirt/network.c b/ext/libvirt/network.c index 7c77d73..c250d7d 100644 --- a/ext/libvirt/network.c +++ b/ext/libvirt/network.c @@ -269,14 +269,20 @@ static VALUE leases_wrap(VALUE arg) rb_hash_aset(hash, rb_str_new2("expirytime"), LL2NUM(lease->expirytime)); rb_hash_aset(hash, rb_str_new2("type"), INT2NUM(lease ->type)); - rb_hash_aset(hash, rb_str_new2("mac"), rb_str_new2(lease ->mac)); - rb_hash_aset(hash, rb_str_new2("iaid"), rb_str_new2(lease ->iaid)); + if (lease->mac) + rb_hash_aset(hash, rb_str_new2("mac"), rb_str_new2(lease ->mac)); + if (lease->iaid) + rb_hash_aset(hash, rb_str_new2("iaid"), rb_str_new2(lease->iaid)); rb_hash_aset(hash, rb_str_new2("ipaddr"), rb_str_new2(lease ->ipaddr)); rb_hash_aset(hash, rb_str_new2("prefix"), UINT2NUM(lease ->prefix)); - rb_hash_aset(hash, rb_str_new2("hostname"), - rb_str_new2(lease->hostname)); - rb_hash_aset(hash, rb_str_new2("clientid"), - rb_str_new2(lease->clientid)); + if (lease->hostname) { + rb_hash_aset(hash, rb_str_new2("hostname"), + rb_str_new2(lease->hostname)); + } + if (lease->clientid) { + rb_hash_aset(hash, rb_str_new2("clientid"), + rb_str_new2(lease->clientid)); + }
rb_ary_store(result, i, hash); } -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Hi! On Thu, Jan 14, 2016 at 2:56 PM, Dan Williams <dcbw@redhat.com> wrote:
On Thu, 2016-01-14 at 13:19 -0500, Laine Stump wrote:
On 01/14/2016 11:01 AM, Dan Williams wrote:
On Thu, 2016-01-07 at 11:12 -0600, Dan Williams wrote:
Not all lease values are mandatory, and when they aren't supplied by the libvirt driver they get set to NULL. That makes rb_str_new2() bail out. Ping? Does this patch look OK or is there anything else I need to do with it? Is the submission procure for ruby-libvirt different than normal libvirt?
As far as I can see, posting to libvir-list is the correct thing for ruby-libvirt patches, I think it's just that very few people use it, so most of us don't feel comfortable ACKing anything for it.
Thanks! I'd bet a lot more people use it than you think, since it's a dependency of vagrant-libvirt by way of fog. So you can't stand up a vagrant machine using libvirt without it...
Dan
It looks like Chris Lalancette is the maintainer and has been the author of nearly every patch in the last couple years, so I'm Cc'ing him to be sure it see it.
For whatever reason, I didn't see your earlier patches. I'll take a look and get back to you. Chris
On Thu, Jan 14, 2016 at 9:38 PM, Chris Lalancette <clalancette@gmail.com> wrote:
Hi!
On Thu, Jan 14, 2016 at 2:56 PM, Dan Williams <dcbw@redhat.com> wrote:
On Thu, 2016-01-14 at 13:19 -0500, Laine Stump wrote:
On 01/14/2016 11:01 AM, Dan Williams wrote:
On Thu, 2016-01-07 at 11:12 -0600, Dan Williams wrote:
Not all lease values are mandatory, and when they aren't supplied by the libvirt driver they get set to NULL. That makes rb_str_new2() bail out. Ping? Does this patch look OK or is there anything else I need to do with it? Is the submission procure for ruby-libvirt different than normal libvirt?
As far as I can see, posting to libvir-list is the correct thing for ruby-libvirt patches, I think it's just that very few people use it, so most of us don't feel comfortable ACKing anything for it.
Thanks! I'd bet a lot more people use it than you think, since it's a dependency of vagrant-libvirt by way of fog. So you can't stand up a vagrant machine using libvirt without it...
Dan
It looks like Chris Lalancette is the maintainer and has been the author of nearly every patch in the last couple years, so I'm Cc'ing him to be sure it see it
For whatever reason, I didn't see your earlier patches. I'll take a look and get back to you.
I've now applied the patch. Thanks for the contribution! Chris
On Thu, 2016-01-14 at 21:44 -0500, Chris Lalancette wrote:
On Thu, Jan 14, 2016 at 9:38 PM, Chris Lalancette <clalancette@gmail. com> wrote:
Hi!
On Thu, Jan 14, 2016 at 2:56 PM, Dan Williams <dcbw@redhat.com> wrote:
On Thu, 2016-01-14 at 13:19 -0500, Laine Stump wrote:
On 01/14/2016 11:01 AM, Dan Williams wrote:
On Thu, 2016-01-07 at 11:12 -0600, Dan Williams wrote:
Not all lease values are mandatory, and when they aren't supplied by the libvirt driver they get set to NULL. That makes rb_str_new2() bail out.
Ping? Does this patch look OK or is there anything else I need to do with it? Is the submission procure for ruby-libvirt different than normal libvirt?
As far as I can see, posting to libvir-list is the correct thing for ruby-libvirt patches, I think it's just that very few people use it, so most of us don't feel comfortable ACKing anything for it.
Thanks! I'd bet a lot more people use it than you think, since it's a dependency of vagrant-libvirt by way of fog. So you can't stand up a vagrant machine using libvirt without it...
Dan
It looks like Chris Lalancette is the maintainer and has been the author of nearly every patch in the last couple years, so I'm Cc'ing him to be sure it see it
For whatever reason, I didn't see your earlier patches. I'll take a look and get back to you.
I've now applied the patch. Thanks for the contribution!
Chris, any chance we could get a new ruby-libvirt point release with the dhcp_leases fix patch in it? The fog-libvirt patches to use that functionality have now landed and people are confused that ruby-libvirt 0.6.0 doesn't have all the necessary fixes. Thanks, Dan
participants (3)
-
Chris Lalancette -
Dan Williams -
Laine Stump