Commit b43c9506 authored by Scott Vokes's avatar Scott Vokes
Browse files

Fix race condition caused by unlocking and relocking during PDU freeing. 

Rather than unlocking and immediately re-locking inside KineticAllocator_FreeItem,
just retain the lock and inform KineticAllocator_FreeItem that it should not
re-lock the mutex. This would be better handled by configuring the mutexes as
recursive/counting, but as we are discussing changing the threading model soon,
it is probably not worth restructuring the mutex initialization at the moment.
parent 32ff32f7

src/lib/kinetic_allocator.c

+20 −7
Original line number Diff line number Diff line
@@ -110,14 +110,19 @@ static void* KineticAllocator_NewItem(KineticList* const list, size_t size) 
    return newItem->data;

}



static void KineticAllocator_FreeItem(KineticList* const list, void* item)

static void KineticAllocator_FreeItem(KineticList* const list, void* item, bool lock)

{

    /* Make locking optional, since the lock may already be owned by the caller. */

    if (lock) {

        KINETIC_LIST_LOCK(list);

    }

    KineticListItem* cur = list->start;

    while (cur->data != item) {

        if (cur->next == NULL) {

            LOG1("  Reached end of list before finding item to free!");

            if (lock) {

                KINETIC_LIST_UNLOCK(list);

            }

            return;

        }

        else {
@@ -171,8 +176,10 @@ static void KineticAllocator_FreeItem(KineticList* const list, void* item) 
        free(cur);

        cur = NULL;

    }

    if (lock) {

        KINETIC_LIST_UNLOCK(list);

    }

}



static void KineticAllocator_FreeList(KineticList* const list)

{
@@ -242,8 +249,14 @@ void KineticAllocator_FreePDU(KineticConnection* connection, KineticPDU* pdu) 
        LOG3("Freeing dynamically allocated protobuf");

        KineticProto_Message__free_unpacked(pdu->proto, NULL);

    };

    

    /* TODO: We can't unlock until the function below completes, but it

     *     normally also tries to lock, so pass in a flag indicating

     *     we already have it locked. The way that the mutexes are

     *     currently initialized makes adding an attribute of

     *     PTHREAD_MUTEX_RECURSIVE significantly more trouble. */

    KineticAllocator_FreeItem(&connection->pdus, (void*)pdu, false);

    KINETIC_LIST_UNLOCK(&connection->pdus);

    KineticAllocator_FreeItem(&connection->pdus, (void*)pdu);

    LOGF3("Freed PDU (0x%0llX) on connection (0x%0llX)", pdu, connection);

}
@@ -320,7 +333,7 @@ void KineticAllocator_FreeOperation(KineticConnection* const connection, Kinetic 
        KineticAllocator_FreePDU(connection, operation->response);

    }

    pthread_mutex_destroy(&operation->timeoutTimeMutex);

    KineticAllocator_FreeItem(&connection->operations, (void*)operation);

    KineticAllocator_FreeItem(&connection->operations, (void*)operation, true);

    LOGF3("Freed operation (0x%0llX) on connection (0x%0llX)", operation, connection);

}
@@ -360,7 +373,7 @@ void KineticAllocator_FreeAllOperations(KineticConnection* const connection) 
                }



                current = current->next;

                KineticAllocator_FreeItem(&connection->operations, (void*)op);

                KineticAllocator_FreeItem(&connection->operations, (void*)op, true);

            }

        }

    }