subject:"\[libvirt\] \[PATCH\] events\: Avoid double free possibility on remote call failure"

Re: [libvirt] [PATCH] events: Avoid double free possibility on remote call failure

2017-06-25 Thread John Ferlan



On 06/25/2017 04:51 AM, Michal Privoznik wrote:
> On 06/25/2017 12:34 AM, John Ferlan wrote:
>>
>>
>> On 06/24/2017 01:13 PM, Michal Privoznik wrote:
>>> On 06/24/2017 01:26 PM, John Ferlan wrote:


 On 06/23/2017 05:39 AM, Michal Privoznik wrote:
> On 06/23/2017 12:10 AM, John Ferlan wrote:
>> If a remote call fails during event registration (more than likely from
>> a network failure or remote libvirtd restart timed just right), then when
>> calling the virObjectEventStateDeregisterID we don't want to call the
>> registered @freecb function because that breaks our contract that we
>> would only call it after succesfully returning.  If the @freecb routine
>> were called, it could result in a double free from properly coded
>> applications that free their opaque data on failure to register, as seen
>> in the following details:
>>
>> Program terminated with signal 6, Aborted.
>> #0  0x7fc45cba15d7 in raise
>> #1  0x7fc45cba2cc8 in abort
>> #2  0x7fc45cbe12f7 in __libc_message
>> #3  0x7fc45cbe86d3 in _int_free
>> #4  0x7fc45d8d292c in PyDict_Fini
>> #5  0x7fc45d94f46a in Py_Finalize
>> #6  0x7fc45d960735 in Py_Main
>> #7  0x7fc45cb8daf5 in __libc_start_main
>> #8  0x00400721 in _start
>>
>> The double dereference of 'pyobj_cbData' is triggered in the following 
>> way:
>>
>> (1) libvirt_virConnectDomainEventRegisterAny is invoked.
>> (2) the event is successfully added to the event callback list
>> (virDomainEventStateRegisterClient in
>> remoteConnectDomainEventRegisterAny returns 1 which means ok).
>> (3) when function remoteConnectDomainEventRegisterAny is hit,
>> network connection disconnected coincidently (or libvirtd is
>> restarted) in the context of function 'call' then the connection
>> is lost and the function 'call' failed, the branch
>> virObjectEventStateDeregisterID is therefore taken.
>> (4) 'pyobj_conn' is dereferenced the 1st time in
>> libvirt_virConnectDomainEventFreeFunc.
>> (5) 'pyobj_cbData' (refered to pyobj_conn) is dereferenced the
>>  2nd time in libvirt_virConnectDomainEventRegisterAny.
>> (6) the double free error is triggered.
>>
>> Resolve this by adding an @inhibitFreeCb boolean in order to avoid 
>> calling
>> freecb in virObjectEventStateDeregisterID for any remote call failure in
>> a remoteConnect*EventRegister* API. For remoteConnect*EventDeregister* 
>> calls,
>> the passed value would be false indicating they should run the freecb if 
>> it
>> exists.
>>
>> Patch based on the investigation and initial patch posted by:
>> fangying .
>>
>> Signed-off-by: John Ferlan 
>> ---
>>   Initial patch found at:
>>
>>   https://www.redhat.com/archives/libvir-list/2017-June/msg00039.html
>>
>>   based on feedback from Daniel Berrange to a previous posting:
>>
>>   https://www.redhat.com/archives/libvir-list/2017-May/msg01089.html
>>
>>   Since no new patch was posted, I posted my idea from review for
>>   consideration.
>>
>>  src/bhyve/bhyve_driver.c |  2 +-
>>  src/conf/domain_event.c  |  2 +-
>>  src/conf/object_event.c  | 23 +--
>>  src/conf/object_event.h  |  3 ++-
>>  src/libxl/libxl_driver.c |  2 +-
>>  src/lxc/lxc_driver.c |  2 +-
>>  src/network/bridge_driver.c  |  2 +-
>>  src/node_device/node_device_driver.c |  2 +-
>>  src/qemu/qemu_driver.c   |  4 ++--
>>  src/remote/remote_driver.c   | 32 
>> 
>>  src/secret/secret_driver.c   |  2 +-
>>  src/storage/storage_driver.c |  2 +-
>>  src/test/test_driver.c   |  8 
>>  src/uml/uml_driver.c |  2 +-
>>  src/vz/vz_driver.c   |  2 +-
>>  src/xen/xen_driver.c |  2 +-
>>  16 files changed, 52 insertions(+), 40 deletions(-)
>>
>> diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
>> index ed2221a..6722dc4 100644
>> --- a/src/bhyve/bhyve_driver.c
>> +++ b/src/bhyve/bhyve_driver.c
>> @@ -1503,7 +1503,7 @@ bhyveConnectDomainEventDeregisterAny(virConnectPtr 
>> conn,
>>  
>>  if (virObjectEventStateDeregisterID(conn,
>>  privconn->domainEventState,
>> -callbackID) < 0)
>> +callbackID, false) < 0)
>>  return -1;
>>  
>>  return 0;
>> diff --git

Re: [libvirt] [PATCH] events: Avoid double free possibility on remote call failure

2017-06-25 Thread Michal Privoznik

On 06/25/2017 12:34 AM, John Ferlan wrote:
> 
> 
> On 06/24/2017 01:13 PM, Michal Privoznik wrote:
>> On 06/24/2017 01:26 PM, John Ferlan wrote:
>>>
>>>
>>> On 06/23/2017 05:39 AM, Michal Privoznik wrote:
 On 06/23/2017 12:10 AM, John Ferlan wrote:
> If a remote call fails during event registration (more than likely from
> a network failure or remote libvirtd restart timed just right), then when
> calling the virObjectEventStateDeregisterID we don't want to call the
> registered @freecb function because that breaks our contract that we
> would only call it after succesfully returning.  If the @freecb routine
> were called, it could result in a double free from properly coded
> applications that free their opaque data on failure to register, as seen
> in the following details:
>
> Program terminated with signal 6, Aborted.
> #0  0x7fc45cba15d7 in raise
> #1  0x7fc45cba2cc8 in abort
> #2  0x7fc45cbe12f7 in __libc_message
> #3  0x7fc45cbe86d3 in _int_free
> #4  0x7fc45d8d292c in PyDict_Fini
> #5  0x7fc45d94f46a in Py_Finalize
> #6  0x7fc45d960735 in Py_Main
> #7  0x7fc45cb8daf5 in __libc_start_main
> #8  0x00400721 in _start
>
> The double dereference of 'pyobj_cbData' is triggered in the following 
> way:
>
> (1) libvirt_virConnectDomainEventRegisterAny is invoked.
> (2) the event is successfully added to the event callback list
> (virDomainEventStateRegisterClient in
> remoteConnectDomainEventRegisterAny returns 1 which means ok).
> (3) when function remoteConnectDomainEventRegisterAny is hit,
> network connection disconnected coincidently (or libvirtd is
> restarted) in the context of function 'call' then the connection
> is lost and the function 'call' failed, the branch
> virObjectEventStateDeregisterID is therefore taken.
> (4) 'pyobj_conn' is dereferenced the 1st time in
> libvirt_virConnectDomainEventFreeFunc.
> (5) 'pyobj_cbData' (refered to pyobj_conn) is dereferenced the
>  2nd time in libvirt_virConnectDomainEventRegisterAny.
> (6) the double free error is triggered.
>
> Resolve this by adding an @inhibitFreeCb boolean in order to avoid calling
> freecb in virObjectEventStateDeregisterID for any remote call failure in
> a remoteConnect*EventRegister* API. For remoteConnect*EventDeregister* 
> calls,
> the passed value would be false indicating they should run the freecb if 
> it
> exists.
>
> Patch based on the investigation and initial patch posted by:
> fangying .
>
> Signed-off-by: John Ferlan 
> ---
>   Initial patch found at:
>
>   https://www.redhat.com/archives/libvir-list/2017-June/msg00039.html
>
>   based on feedback from Daniel Berrange to a previous posting:
>
>   https://www.redhat.com/archives/libvir-list/2017-May/msg01089.html
>
>   Since no new patch was posted, I posted my idea from review for
>   consideration.
>
>  src/bhyve/bhyve_driver.c |  2 +-
>  src/conf/domain_event.c  |  2 +-
>  src/conf/object_event.c  | 23 +--
>  src/conf/object_event.h  |  3 ++-
>  src/libxl/libxl_driver.c |  2 +-
>  src/lxc/lxc_driver.c |  2 +-
>  src/network/bridge_driver.c  |  2 +-
>  src/node_device/node_device_driver.c |  2 +-
>  src/qemu/qemu_driver.c   |  4 ++--
>  src/remote/remote_driver.c   | 32 
> 
>  src/secret/secret_driver.c   |  2 +-
>  src/storage/storage_driver.c |  2 +-
>  src/test/test_driver.c   |  8 
>  src/uml/uml_driver.c |  2 +-
>  src/vz/vz_driver.c   |  2 +-
>  src/xen/xen_driver.c |  2 +-
>  16 files changed, 52 insertions(+), 40 deletions(-)
>
> diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
> index ed2221a..6722dc4 100644
> --- a/src/bhyve/bhyve_driver.c
> +++ b/src/bhyve/bhyve_driver.c
> @@ -1503,7 +1503,7 @@ bhyveConnectDomainEventDeregisterAny(virConnectPtr 
> conn,
>  
>  if (virObjectEventStateDeregisterID(conn,
>  privconn->domainEventState,
> -callbackID) < 0)
> +callbackID, false) < 0)
>  return -1;
>  
>  return 0;
> diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c
> index 6e471d7..ff4c602 100644
> --- a/src/conf/domain_event.c
> +++ b/src/conf/domain_event.c
>

Re: [libvirt] [PATCH] events: Avoid double free possibility on remote call failure

2017-06-25 Thread John Ferlan

On 06/24/2017 01:13 PM, Michal Privoznik wrote:
> On 06/24/2017 01:26 PM, John Ferlan wrote:
>>
>>
>> On 06/23/2017 05:39 AM, Michal Privoznik wrote:
>>> On 06/23/2017 12:10 AM, John Ferlan wrote:
 If a remote call fails during event registration (more than likely from
 a network failure or remote libvirtd restart timed just right), then when
 calling the virObjectEventStateDeregisterID we don't want to call the
 registered @freecb function because that breaks our contract that we
 would only call it after succesfully returning.  If the @freecb routine
 were called, it could result in a double free from properly coded
 applications that free their opaque data on failure to register, as seen
 in the following details:

 Program terminated with signal 6, Aborted.
 #0  0x7fc45cba15d7 in raise
 #1  0x7fc45cba2cc8 in abort
 #2  0x7fc45cbe12f7 in __libc_message
 #3  0x7fc45cbe86d3 in _int_free
 #4  0x7fc45d8d292c in PyDict_Fini
 #5  0x7fc45d94f46a in Py_Finalize
 #6  0x7fc45d960735 in Py_Main
 #7  0x7fc45cb8daf5 in __libc_start_main
 #8  0x00400721 in _start

 The double dereference of 'pyobj_cbData' is triggered in the following way:

 (1) libvirt_virConnectDomainEventRegisterAny is invoked.
 (2) the event is successfully added to the event callback list
 (virDomainEventStateRegisterClient in
 remoteConnectDomainEventRegisterAny returns 1 which means ok).
 (3) when function remoteConnectDomainEventRegisterAny is hit,
 network connection disconnected coincidently (or libvirtd is
 restarted) in the context of function 'call' then the connection
 is lost and the function 'call' failed, the branch
 virObjectEventStateDeregisterID is therefore taken.
 (4) 'pyobj_conn' is dereferenced the 1st time in
 libvirt_virConnectDomainEventFreeFunc.
 (5) 'pyobj_cbData' (refered to pyobj_conn) is dereferenced the
  2nd time in libvirt_virConnectDomainEventRegisterAny.
 (6) the double free error is triggered.

 Resolve this by adding an @inhibitFreeCb boolean in order to avoid calling
 freecb in virObjectEventStateDeregisterID for any remote call failure in
 a remoteConnect*EventRegister* API. For remoteConnect*EventDeregister* 
 calls,
 the passed value would be false indicating they should run the freecb if it
 exists.

 Patch based on the investigation and initial patch posted by:
 fangying .

 Signed-off-by: John Ferlan 
 ---
   Initial patch found at:

   https://www.redhat.com/archives/libvir-list/2017-June/msg00039.html

   based on feedback from Daniel Berrange to a previous posting:

   https://www.redhat.com/archives/libvir-list/2017-May/msg01089.html

   Since no new patch was posted, I posted my idea from review for
   consideration.

  src/bhyve/bhyve_driver.c |  2 +-
  src/conf/domain_event.c  |  2 +-
  src/conf/object_event.c  | 23 +--
  src/conf/object_event.h  |  3 ++-
  src/libxl/libxl_driver.c |  2 +-
  src/lxc/lxc_driver.c |  2 +-
  src/network/bridge_driver.c  |  2 +-
  src/node_device/node_device_driver.c |  2 +-
  src/qemu/qemu_driver.c   |  4 ++--
  src/remote/remote_driver.c   | 32 
  src/secret/secret_driver.c   |  2 +-
  src/storage/storage_driver.c |  2 +-
  src/test/test_driver.c   |  8 
  src/uml/uml_driver.c |  2 +-
  src/vz/vz_driver.c   |  2 +-
  src/xen/xen_driver.c |  2 +-
  16 files changed, 52 insertions(+), 40 deletions(-)

 diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
 index ed2221a..6722dc4 100644
 --- a/src/bhyve/bhyve_driver.c
 +++ b/src/bhyve/bhyve_driver.c
 @@ -1503,7 +1503,7 @@ bhyveConnectDomainEventDeregisterAny(virConnectPtr 
 conn,

  if (virObjectEventStateDeregisterID(conn,
  privconn->domainEventState,
 -callbackID) < 0)
 +callbackID, false) < 0)
  return -1;

  return 0;
 diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c
 index 6e471d7..ff4c602 100644
 --- a/src/conf/domain_event.c
 +++ b/src/conf/domain_event.c
 @@ -2301,7 +2301,7 @@ virDomainEventStateDeregister(virConnectPtr conn,
 NULL);
  if (callbackID < 0)

Re: [libvirt] [PATCH] events: Avoid double free possibility on remote call failure

2017-06-25 Thread Michal Privoznik

On 06/24/2017 01:26 PM, John Ferlan wrote:
> 
> 
> On 06/23/2017 05:39 AM, Michal Privoznik wrote:
>> On 06/23/2017 12:10 AM, John Ferlan wrote:
>>> If a remote call fails during event registration (more than likely from
>>> a network failure or remote libvirtd restart timed just right), then when
>>> calling the virObjectEventStateDeregisterID we don't want to call the
>>> registered @freecb function because that breaks our contract that we
>>> would only call it after succesfully returning.  If the @freecb routine
>>> were called, it could result in a double free from properly coded
>>> applications that free their opaque data on failure to register, as seen
>>> in the following details:
>>>
>>> Program terminated with signal 6, Aborted.
>>> #0  0x7fc45cba15d7 in raise
>>> #1  0x7fc45cba2cc8 in abort
>>> #2  0x7fc45cbe12f7 in __libc_message
>>> #3  0x7fc45cbe86d3 in _int_free
>>> #4  0x7fc45d8d292c in PyDict_Fini
>>> #5  0x7fc45d94f46a in Py_Finalize
>>> #6  0x7fc45d960735 in Py_Main
>>> #7  0x7fc45cb8daf5 in __libc_start_main
>>> #8  0x00400721 in _start
>>>
>>> The double dereference of 'pyobj_cbData' is triggered in the following way:
>>>
>>> (1) libvirt_virConnectDomainEventRegisterAny is invoked.
>>> (2) the event is successfully added to the event callback list
>>> (virDomainEventStateRegisterClient in
>>> remoteConnectDomainEventRegisterAny returns 1 which means ok).
>>> (3) when function remoteConnectDomainEventRegisterAny is hit,
>>> network connection disconnected coincidently (or libvirtd is
>>> restarted) in the context of function 'call' then the connection
>>> is lost and the function 'call' failed, the branch
>>> virObjectEventStateDeregisterID is therefore taken.
>>> (4) 'pyobj_conn' is dereferenced the 1st time in
>>> libvirt_virConnectDomainEventFreeFunc.
>>> (5) 'pyobj_cbData' (refered to pyobj_conn) is dereferenced the
>>>  2nd time in libvirt_virConnectDomainEventRegisterAny.
>>> (6) the double free error is triggered.
>>>
>>> Resolve this by adding an @inhibitFreeCb boolean in order to avoid calling
>>> freecb in virObjectEventStateDeregisterID for any remote call failure in
>>> a remoteConnect*EventRegister* API. For remoteConnect*EventDeregister* 
>>> calls,
>>> the passed value would be false indicating they should run the freecb if it
>>> exists.
>>>
>>> Patch based on the investigation and initial patch posted by:
>>> fangying .
>>>
>>> Signed-off-by: John Ferlan 
>>> ---
>>>   Initial patch found at:
>>>
>>>   https://www.redhat.com/archives/libvir-list/2017-June/msg00039.html
>>>
>>>   based on feedback from Daniel Berrange to a previous posting:
>>>
>>>   https://www.redhat.com/archives/libvir-list/2017-May/msg01089.html
>>>
>>>   Since no new patch was posted, I posted my idea from review for
>>>   consideration.
>>>
>>>  src/bhyve/bhyve_driver.c |  2 +-
>>>  src/conf/domain_event.c  |  2 +-
>>>  src/conf/object_event.c  | 23 +--
>>>  src/conf/object_event.h  |  3 ++-
>>>  src/libxl/libxl_driver.c |  2 +-
>>>  src/lxc/lxc_driver.c |  2 +-
>>>  src/network/bridge_driver.c  |  2 +-
>>>  src/node_device/node_device_driver.c |  2 +-
>>>  src/qemu/qemu_driver.c   |  4 ++--
>>>  src/remote/remote_driver.c   | 32 
>>>  src/secret/secret_driver.c   |  2 +-
>>>  src/storage/storage_driver.c |  2 +-
>>>  src/test/test_driver.c   |  8 
>>>  src/uml/uml_driver.c |  2 +-
>>>  src/vz/vz_driver.c   |  2 +-
>>>  src/xen/xen_driver.c |  2 +-
>>>  16 files changed, 52 insertions(+), 40 deletions(-)
>>>
>>> diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
>>> index ed2221a..6722dc4 100644
>>> --- a/src/bhyve/bhyve_driver.c
>>> +++ b/src/bhyve/bhyve_driver.c
>>> @@ -1503,7 +1503,7 @@ bhyveConnectDomainEventDeregisterAny(virConnectPtr 
>>> conn,
>>>  
>>>  if (virObjectEventStateDeregisterID(conn,
>>>  privconn->domainEventState,
>>> -callbackID) < 0)
>>> +callbackID, false) < 0)
>>>  return -1;
>>>  
>>>  return 0;
>>> diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c
>>> index 6e471d7..ff4c602 100644
>>> --- a/src/conf/domain_event.c
>>> +++ b/src/conf/domain_event.c
>>> @@ -2301,7 +2301,7 @@ virDomainEventStateDeregister(virConnectPtr conn,
>>> NULL);
>>>  if (callbackID < 0)
>>>  return -1;
>>> -return virObjectEventStateDeregisterID(conn, state, callbackID);
>>> +return virObjectEventStateDeregisterID(conn, state,

Re: [libvirt] [PATCH] events: Avoid double free possibility on remote call failure

2017-06-24 Thread John Ferlan



On 06/23/2017 05:39 AM, Michal Privoznik wrote:
> On 06/23/2017 12:10 AM, John Ferlan wrote:
>> If a remote call fails during event registration (more than likely from
>> a network failure or remote libvirtd restart timed just right), then when
>> calling the virObjectEventStateDeregisterID we don't want to call the
>> registered @freecb function because that breaks our contract that we
>> would only call it after succesfully returning.  If the @freecb routine
>> were called, it could result in a double free from properly coded
>> applications that free their opaque data on failure to register, as seen
>> in the following details:
>>
>> Program terminated with signal 6, Aborted.
>> #0  0x7fc45cba15d7 in raise
>> #1  0x7fc45cba2cc8 in abort
>> #2  0x7fc45cbe12f7 in __libc_message
>> #3  0x7fc45cbe86d3 in _int_free
>> #4  0x7fc45d8d292c in PyDict_Fini
>> #5  0x7fc45d94f46a in Py_Finalize
>> #6  0x7fc45d960735 in Py_Main
>> #7  0x7fc45cb8daf5 in __libc_start_main
>> #8  0x00400721 in _start
>>
>> The double dereference of 'pyobj_cbData' is triggered in the following way:
>>
>> (1) libvirt_virConnectDomainEventRegisterAny is invoked.
>> (2) the event is successfully added to the event callback list
>> (virDomainEventStateRegisterClient in
>> remoteConnectDomainEventRegisterAny returns 1 which means ok).
>> (3) when function remoteConnectDomainEventRegisterAny is hit,
>> network connection disconnected coincidently (or libvirtd is
>> restarted) in the context of function 'call' then the connection
>> is lost and the function 'call' failed, the branch
>> virObjectEventStateDeregisterID is therefore taken.
>> (4) 'pyobj_conn' is dereferenced the 1st time in
>> libvirt_virConnectDomainEventFreeFunc.
>> (5) 'pyobj_cbData' (refered to pyobj_conn) is dereferenced the
>>  2nd time in libvirt_virConnectDomainEventRegisterAny.
>> (6) the double free error is triggered.
>>
>> Resolve this by adding an @inhibitFreeCb boolean in order to avoid calling
>> freecb in virObjectEventStateDeregisterID for any remote call failure in
>> a remoteConnect*EventRegister* API. For remoteConnect*EventDeregister* calls,
>> the passed value would be false indicating they should run the freecb if it
>> exists.
>>
>> Patch based on the investigation and initial patch posted by:
>> fangying .
>>
>> Signed-off-by: John Ferlan 
>> ---
>>   Initial patch found at:
>>
>>   https://www.redhat.com/archives/libvir-list/2017-June/msg00039.html
>>
>>   based on feedback from Daniel Berrange to a previous posting:
>>
>>   https://www.redhat.com/archives/libvir-list/2017-May/msg01089.html
>>
>>   Since no new patch was posted, I posted my idea from review for
>>   consideration.
>>
>>  src/bhyve/bhyve_driver.c |  2 +-
>>  src/conf/domain_event.c  |  2 +-
>>  src/conf/object_event.c  | 23 +--
>>  src/conf/object_event.h  |  3 ++-
>>  src/libxl/libxl_driver.c |  2 +-
>>  src/lxc/lxc_driver.c |  2 +-
>>  src/network/bridge_driver.c  |  2 +-
>>  src/node_device/node_device_driver.c |  2 +-
>>  src/qemu/qemu_driver.c   |  4 ++--
>>  src/remote/remote_driver.c   | 32 
>>  src/secret/secret_driver.c   |  2 +-
>>  src/storage/storage_driver.c |  2 +-
>>  src/test/test_driver.c   |  8 
>>  src/uml/uml_driver.c |  2 +-
>>  src/vz/vz_driver.c   |  2 +-
>>  src/xen/xen_driver.c |  2 +-
>>  16 files changed, 52 insertions(+), 40 deletions(-)
>>
>> diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
>> index ed2221a..6722dc4 100644
>> --- a/src/bhyve/bhyve_driver.c
>> +++ b/src/bhyve/bhyve_driver.c
>> @@ -1503,7 +1503,7 @@ bhyveConnectDomainEventDeregisterAny(virConnectPtr 
>> conn,
>>  
>>  if (virObjectEventStateDeregisterID(conn,
>>  privconn->domainEventState,
>> -callbackID) < 0)
>> +callbackID, false) < 0)
>>  return -1;
>>  
>>  return 0;
>> diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c
>> index 6e471d7..ff4c602 100644
>> --- a/src/conf/domain_event.c
>> +++ b/src/conf/domain_event.c
>> @@ -2301,7 +2301,7 @@ virDomainEventStateDeregister(virConnectPtr conn,
>> NULL);
>>  if (callbackID < 0)
>>  return -1;
>> -return virObjectEventStateDeregisterID(conn, state, callbackID);
>> +return virObjectEventStateDeregisterID(conn, state, callbackID, false);
> 
> Why is this false? virDomainEventStateDeregister is called directly from
> public API implementations. For instance from
>

Re: [libvirt] [PATCH] events: Avoid double free possibility on remote call failure

2017-06-23 Thread Michal Privoznik

On 06/23/2017 12:10 AM, John Ferlan wrote:
> If a remote call fails during event registration (more than likely from
> a network failure or remote libvirtd restart timed just right), then when
> calling the virObjectEventStateDeregisterID we don't want to call the
> registered @freecb function because that breaks our contract that we
> would only call it after succesfully returning.  If the @freecb routine
> were called, it could result in a double free from properly coded
> applications that free their opaque data on failure to register, as seen
> in the following details:
> 
> Program terminated with signal 6, Aborted.
> #0  0x7fc45cba15d7 in raise
> #1  0x7fc45cba2cc8 in abort
> #2  0x7fc45cbe12f7 in __libc_message
> #3  0x7fc45cbe86d3 in _int_free
> #4  0x7fc45d8d292c in PyDict_Fini
> #5  0x7fc45d94f46a in Py_Finalize
> #6  0x7fc45d960735 in Py_Main
> #7  0x7fc45cb8daf5 in __libc_start_main
> #8  0x00400721 in _start
> 
> The double dereference of 'pyobj_cbData' is triggered in the following way:
> 
> (1) libvirt_virConnectDomainEventRegisterAny is invoked.
> (2) the event is successfully added to the event callback list
> (virDomainEventStateRegisterClient in
> remoteConnectDomainEventRegisterAny returns 1 which means ok).
> (3) when function remoteConnectDomainEventRegisterAny is hit,
> network connection disconnected coincidently (or libvirtd is
> restarted) in the context of function 'call' then the connection
> is lost and the function 'call' failed, the branch
> virObjectEventStateDeregisterID is therefore taken.
> (4) 'pyobj_conn' is dereferenced the 1st time in
> libvirt_virConnectDomainEventFreeFunc.
> (5) 'pyobj_cbData' (refered to pyobj_conn) is dereferenced the
>  2nd time in libvirt_virConnectDomainEventRegisterAny.
> (6) the double free error is triggered.
> 
> Resolve this by adding an @inhibitFreeCb boolean in order to avoid calling
> freecb in virObjectEventStateDeregisterID for any remote call failure in
> a remoteConnect*EventRegister* API. For remoteConnect*EventDeregister* calls,
> the passed value would be false indicating they should run the freecb if it
> exists.
> 
> Patch based on the investigation and initial patch posted by:
> fangying .
> 
> Signed-off-by: John Ferlan 
> ---
>   Initial patch found at:
> 
>   https://www.redhat.com/archives/libvir-list/2017-June/msg00039.html
> 
>   based on feedback from Daniel Berrange to a previous posting:
> 
>   https://www.redhat.com/archives/libvir-list/2017-May/msg01089.html
> 
>   Since no new patch was posted, I posted my idea from review for
>   consideration.
> 
>  src/bhyve/bhyve_driver.c |  2 +-
>  src/conf/domain_event.c  |  2 +-
>  src/conf/object_event.c  | 23 +--
>  src/conf/object_event.h  |  3 ++-
>  src/libxl/libxl_driver.c |  2 +-
>  src/lxc/lxc_driver.c |  2 +-
>  src/network/bridge_driver.c  |  2 +-
>  src/node_device/node_device_driver.c |  2 +-
>  src/qemu/qemu_driver.c   |  4 ++--
>  src/remote/remote_driver.c   | 32 
>  src/secret/secret_driver.c   |  2 +-
>  src/storage/storage_driver.c |  2 +-
>  src/test/test_driver.c   |  8 
>  src/uml/uml_driver.c |  2 +-
>  src/vz/vz_driver.c   |  2 +-
>  src/xen/xen_driver.c |  2 +-
>  16 files changed, 52 insertions(+), 40 deletions(-)
> 
> diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
> index ed2221a..6722dc4 100644
> --- a/src/bhyve/bhyve_driver.c
> +++ b/src/bhyve/bhyve_driver.c
> @@ -1503,7 +1503,7 @@ bhyveConnectDomainEventDeregisterAny(virConnectPtr conn,
>  
>  if (virObjectEventStateDeregisterID(conn,
>  privconn->domainEventState,
> -callbackID) < 0)
> +callbackID, false) < 0)
>  return -1;
>  
>  return 0;
> diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c
> index 6e471d7..ff4c602 100644
> --- a/src/conf/domain_event.c
> +++ b/src/conf/domain_event.c
> @@ -2301,7 +2301,7 @@ virDomainEventStateDeregister(virConnectPtr conn,
> NULL);
>  if (callbackID < 0)
>  return -1;
> -return virObjectEventStateDeregisterID(conn, state, callbackID);
> +return virObjectEventStateDeregisterID(conn, state, callbackID, false);

Why is this false? virDomainEventStateDeregister is called directly from
public API implementations. For instance from
qemuConnectDomainEventDeregister(). Don't we want to run free callback then?

>  }
>  

Michal

--
libvir-list mailing list
libvir-list@redhat.com

[libvirt] [PATCH] events: Avoid double free possibility on remote call failure

2017-06-22 Thread John Ferlan

If a remote call fails during event registration (more than likely from
a network failure or remote libvirtd restart timed just right), then when
calling the virObjectEventStateDeregisterID we don't want to call the
registered @freecb function because that breaks our contract that we
would only call it after succesfully returning.  If the @freecb routine
were called, it could result in a double free from properly coded
applications that free their opaque data on failure to register, as seen
in the following details:

Program terminated with signal 6, Aborted.
#0  0x7fc45cba15d7 in raise
#1  0x7fc45cba2cc8 in abort
#2  0x7fc45cbe12f7 in __libc_message
#3  0x7fc45cbe86d3 in _int_free
#4  0x7fc45d8d292c in PyDict_Fini
#5  0x7fc45d94f46a in Py_Finalize
#6  0x7fc45d960735 in Py_Main
#7  0x7fc45cb8daf5 in __libc_start_main
#8  0x00400721 in _start

The double dereference of 'pyobj_cbData' is triggered in the following way:

(1) libvirt_virConnectDomainEventRegisterAny is invoked.
(2) the event is successfully added to the event callback list
(virDomainEventStateRegisterClient in
remoteConnectDomainEventRegisterAny returns 1 which means ok).
(3) when function remoteConnectDomainEventRegisterAny is hit,
network connection disconnected coincidently (or libvirtd is
restarted) in the context of function 'call' then the connection
is lost and the function 'call' failed, the branch
virObjectEventStateDeregisterID is therefore taken.
(4) 'pyobj_conn' is dereferenced the 1st time in
libvirt_virConnectDomainEventFreeFunc.
(5) 'pyobj_cbData' (refered to pyobj_conn) is dereferenced the
 2nd time in libvirt_virConnectDomainEventRegisterAny.
(6) the double free error is triggered.

Resolve this by adding an @inhibitFreeCb boolean in order to avoid calling
freecb in virObjectEventStateDeregisterID for any remote call failure in
a remoteConnect*EventRegister* API. For remoteConnect*EventDeregister* calls,
the passed value would be false indicating they should run the freecb if it
exists.

Patch based on the investigation and initial patch posted by:
fangying .

Signed-off-by: John Ferlan 
---
  Initial patch found at:

  https://www.redhat.com/archives/libvir-list/2017-June/msg00039.html

  based on feedback from Daniel Berrange to a previous posting:

  https://www.redhat.com/archives/libvir-list/2017-May/msg01089.html

  Since no new patch was posted, I posted my idea from review for
  consideration.

 src/bhyve/bhyve_driver.c |  2 +-
 src/conf/domain_event.c  |  2 +-
 src/conf/object_event.c  | 23 +--
 src/conf/object_event.h  |  3 ++-
 src/libxl/libxl_driver.c |  2 +-
 src/lxc/lxc_driver.c |  2 +-
 src/network/bridge_driver.c  |  2 +-
 src/node_device/node_device_driver.c |  2 +-
 src/qemu/qemu_driver.c   |  4 ++--
 src/remote/remote_driver.c   | 32 
 src/secret/secret_driver.c   |  2 +-
 src/storage/storage_driver.c |  2 +-
 src/test/test_driver.c   |  8 
 src/uml/uml_driver.c |  2 +-
 src/vz/vz_driver.c   |  2 +-
 src/xen/xen_driver.c |  2 +-
 16 files changed, 52 insertions(+), 40 deletions(-)

diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index ed2221a..6722dc4 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -1503,7 +1503,7 @@ bhyveConnectDomainEventDeregisterAny(virConnectPtr conn,
 
 if (virObjectEventStateDeregisterID(conn,
 privconn->domainEventState,
-callbackID) < 0)
+callbackID, false) < 0)
 return -1;
 
 return 0;
diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c
index 6e471d7..ff4c602 100644
--- a/src/conf/domain_event.c
+++ b/src/conf/domain_event.c
@@ -2301,7 +2301,7 @@ virDomainEventStateDeregister(virConnectPtr conn,
NULL);
 if (callbackID < 0)
 return -1;
-return virObjectEventStateDeregisterID(conn, state, callbackID);
+return virObjectEventStateDeregisterID(conn, state, callbackID, false);
 }
 
 
diff --git a/src/conf/object_event.c b/src/conf/object_event.c
index e5f942f..5e944af 100644
--- a/src/conf/object_event.c
+++ b/src/conf/object_event.c
@@ -234,13 +234,15 @@ virObjectEventCallbackListCount(virConnectPtr conn,
  * @conn: pointer to the connection
  * @cbList: the list
  * @callback: the callback to remove
+ * @inhibitFreeCb: Inhibit calling the freecb
  *
  * Internal function to remove a callback from a virObjectEventCallbackListPtr
  */
 static int

Re: [libvirt] [PATCH] events: Avoid double free possibility on remote call failure

Re: [libvirt] [PATCH] events: Avoid double free possibility on remote call failure

Re: [libvirt] [PATCH] events: Avoid double free possibility on remote call failure

Re: [libvirt] [PATCH] events: Avoid double free possibility on remote call failure

Re: [libvirt] [PATCH] events: Avoid double free possibility on remote call failure

Re: [libvirt] [PATCH] events: Avoid double free possibility on remote call failure

[libvirt] [PATCH] events: Avoid double free possibility on remote call failure

7 matches

Site Navigation

Mail list logo

Footer information