Fix reference counting in get_attached_entry (again)

When get_attached_entry() encounters entry that would be expired, it
needs to get reference to the entry before calling maybe_expire_entry(),
so the ADB entry doesn't get destroyed inside the its own lock.

This creeped into the code base again during review, so I am adding
an extra comment to prevent this.

diff --git a/lib/dns/adb.c b/lib/dns/adb.c
index 0454105c14d592030a6b2975c5365b61a0f4c0a1..d105107f95f494ea69479116c0b7c7e3d22c2f23 100644 (file)
--- a/lib/dns/adb.c
+++ b/lib/dns/adb.c
@@ -1368,12 +1368,14 @@ get_attached_and_locked_name(dns_adb_t *adb, const dns_name_t *name,
                                         adbname);
                INSIST(result == ISC_R_SUCCESS);
 
+               dns_adbname_ref(adbname);
                LOCK(&adbname->lock); /* Must be unlocked by the caller */
                adbname->last_used = now;
 
                ISC_LIST_PREPEND(adb->names_lru, adbname, link);
                break;
        case ISC_R_SUCCESS:
+               dns_adbname_ref(adbname);
                LOCK(&adbname->lock); /* Must be unlocked by the caller */
                if (adbname->last_used + ADB_CACHE_MINIMUM <= last_update) {
                        adbname->last_used = now;
@@ -1386,7 +1388,6 @@ get_attached_and_locked_name(dns_adb_t *adb, const dns_name_t *name,
                UNREACHABLE();
        }
 
-       dns_adbname_ref(adbname);
        /*
         * The refcount is now 2 and the final detach will happen in
         * expire_name() - the unused adbname stored in the hashtable and lru
@@ -1436,6 +1437,7 @@ get_attached_and_locked_entry(dns_adb_t *adb, isc_stdtime_t now,
                                         sizeof(adbentry->sockaddr), adbentry);
                INSIST(result == ISC_R_SUCCESS);
 
+               dns_adbentry_ref(adbentry);
                LOCK(&adbentry->lock); /* Must be unlocked by the caller */
                adbentry->last_used = now;
 
@@ -1443,6 +1445,11 @@ get_attached_and_locked_entry(dns_adb_t *adb, isc_stdtime_t now,
                break;
        }
        case ISC_R_SUCCESS:
+               /*
+                * The dns_adbentry_ref() must stay here before trying to expire
+                * the ADB entry, so it is not destroyed under the lock.
+                */
+               dns_adbentry_ref(adbentry);
                LOCK(&adbentry->lock); /* Must be unlocked by the caller */
                if (maybe_expire_entry(adbentry, now)) {
                        UNLOCK(&adbentry->lock);
@@ -1460,8 +1467,6 @@ get_attached_and_locked_entry(dns_adb_t *adb, isc_stdtime_t now,
                UNREACHABLE();
        }
 
-       dns_adbentry_ref(adbentry);
-
        UNLOCK(&adb->entries_lock);
 
        return (adbentry);