[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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.



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]