[libvirt] [PATCH] Update MAC address in virInterface objects when needed.

Laine Stump laine at laine.org
Tue Jul 21 01:24:11 UTC 2009


On 07/20/2009 03:22 PM, Daniel Veillard wrote:
> On Mon, Jul 20, 2009 at 07:44:43PM +0100, Daniel P. Berrange wrote:
>   
>> On Mon, Jul 20, 2009 at 01:42:04PM -0400, Laine Stump wrote:
>>     
>>> MAC address of a particular interface may change over time, and the
>>> reduced virInterface object (which contains just name and mac) needs
>>> to reflect these changes.
>>> ---
>>>  src/datatypes.c |   24 ++++++++++++++++--------
>>>  1 files changed, 16 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/src/datatypes.c b/src/datatypes.c
>>> index a8bffd2..a0d027c 100644
>>> --- a/src/datatypes.c
>>> +++ b/src/datatypes.c
>>> @@ -516,9 +516,10 @@ virUnrefNetwork(virNetworkPtr network) {
>>>   * @mac: pointer to the mac
>>>   *
>>>   * Lookup if the interface is already registered for that connection,
>>> - * if yes return a new pointer to it, if no allocate a new structure,
>>> - * and register it in the table. In any case a corresponding call to
>>> - * virUnrefInterface() is needed to not leak data.
>>> + * if yes return a new pointer to it (possibly updating the MAC
>>> + * address), if no allocate a new structure, and register it in the
>>> + * table. In any case a corresponding call to virUnrefInterface() is
>>> + * needed to not leak data.
>>>   *
>>>   * Returns a pointer to the interface, or NULL in case of failure
>>>   */
>>> @@ -532,11 +533,19 @@ virGetInterface(virConnectPtr conn, const char *name, const char *mac) {
>>>      }
>>>      virMutexLock(&conn->lock);
>>>  
>>> -    /* TODO search by MAC first as they are better differentiators */
>>> -
>>>      ret = (virInterfacePtr) virHashLookup(conn->interfaces, name);
>>> -    /* TODO check the MAC */
>>> -    if (ret == NULL) {
>>> +
>>> +    if (ret != NULL) {
>>> +        /* update MAC address if necessary */
>>> +        if ((ret->mac == NULL) || STRNEQ(ret->mac, mac)) {
>>> +            VIR_FREE(ret->mac);
>>> +            ret->mac = strdup(mac);
>>> +            if (ret->mac == NULL) {
>>> +                virReportOOMError(conn);
>>> +                goto error;
>>> +            }
>>> +        }
>>> +    } else {
>>>       
>> There's a small edge case there. the 'ret' object you have
>> there is a cached one, whose handled is already in use by
>> other callers on libvirt public API. So although you are
>> reported an OOM to this caller, other users of this cached
>> object have a dangerous instance witha NULL 'mac' field.
>> Easy solution, don't VIR_FREE the existing mac until you
>> have successfully strdup'd the new one
>>     
>
>   Good point, actually if the size permits writing the new value over
> just avoids a new allocation.
>   

Now that you make me think about it - beyond that, if another thread has 
a pointer to this object, they may have already gotten a copy of the mac 
pointer into a temp variable, and if they then use it after I've freed 
it (and maybe someone else re-uses the same memory) they will get bad data.

So I guess the *real* solution is to always compare the macs, and if 
they don't match create a new object. This will mean that this 
hypothetical "other thread" may be working with out-of-date information 
(it will still have the old mac address, at least for iface-list), but 
at least it will never stomp on someone else's data.





More information about the libvir-list mailing list