[libvirt] [PATCH] events: Avoid double free possibility on remote call failure
John Ferlan
jferlan at redhat.com
Sun Jun 25 12:24:47 UTC 2017
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 0x00007fc45cba15d7 in raise
>>>>>> #1 0x00007fc45cba2cc8 in abort
>>>>>> #2 0x00007fc45cbe12f7 in __libc_message
>>>>>> #3 0x00007fc45cbe86d3 in _int_free
>>>>>> #4 0x00007fc45d8d292c in PyDict_Fini
>>>>>> #5 0x00007fc45d94f46a in Py_Finalize
>>>>>> #6 0x00007fc45d960735 in Py_Main
>>>>>> #7 0x00007fc45cb8daf5 in __libc_start_main
>>>>>> #8 0x0000000000400721 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 <fangying1 at huawei.com>.
>>>>>>
>>>>>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>>>>>> ---
>>>>>> 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?
>>>>>
>>>>
>>>> and the eventual check in virObjectEventCallbackListRemoveID is:
>>>>
>>>> /* inhibiting calling @freecb from error paths in register
>>>> * functions ensures the caller of the register function won't
>>>> * end up with a double free error */
>>>> if (!inhibitFreeCb && cb->freecb)
>>>>
>>>> IOW: Any time called from a Deregister path, we call the free callback;
>>>> however, when called from a RPC call REGISTER path, then we don't want
>>>> to call the free callback because we're returning -1 to the caller and
>>>> the caller will free things.
>>>
>>> Ah, the negated logic confused me. I think I'd be nicer if we've used
>>> boolean in straight logic (= do call) instead of reversed (= don't call).
>>>
>>> What do you think?
>>>
>>
>> I'm chuckling ;-) usually it's the double negatives that get me, but
>> because I was solving the problem of needing to not call the FreeCb, I
>> went with the inhibit option.
>>
>> I can flip-flop though... Instead of inhibitFreeCb, I can change to
>> doFreeCb and repost.
>
> Okay, ACK if you fix that.
>
> Michal
>
OK - I did that and pushed.
Tks,
John
More information about the libvir-list
mailing list