OCEAN-xyz / datum_gateway

Decentralized Alternative Templates for Universal Mining
Other
32 stars 9 forks source link

Fix bugs in datum_queue_free #29

Open jesterhodl opened 2 days ago

jesterhodl commented 2 days ago
  1. q->buffer[1] doesn't seem to be free()'d

  2. q->buffer[0] is duplicated, you probably meant to zero out buffer[1]

  3. shouldn't the unlocking order be reverse of locking order to prevent deadlocks?

wizkid057 commented 2 days ago

buffer[1] cannot be free'd because its allocated in a single call for buffer[0] and then split.

https://github.com/OCEAN-xyz/datum_gateway/blob/a384ff68a165bc8a46bc132366a3c56ce53562d6/src/datum_queue.c#L117

wizkid057 commented 2 days ago

Looking at the locks, in this case the goal is to acquire all locks as write locks, which, once acquired, prevents other threads from acquiring the locks at all until released. There's no contention possible on the ordering of unlocking since the cleanup function has the sole lock at that point.

The unsetting of [0] twice is a bug, though.

Looking more closely, there may be a race issue where the "datum_queue_free" function frees data while a "datum_queue_add_item" tries to get a lock that's already destroyed. Probably need to check if the buffer ptr is not null around here before trying to get the lock.

jesterhodl commented 11 hours ago

I'm reading through the code, but I think I found one more thing. Don't we have a one 0 too much in the condition?

        for (i=0;i<10000000;i++) {
                if (i < 99999990)  // should be 9999999, anything larger doesn't make sense 
// 10000000
// 99999990
wizkid057 commented 11 hours ago

I'm reading through the code, but I think I found one more thing. Don't we have a one 0 too much in the condition?

        for (i=0;i<10000000;i++) {
                if (i < 99999990)  // should be 9999999, anything larger doesn't make sense 
// 10000000
// 99999990

Yep, seems so.

jesterhodl commented 11 hours ago

Is this what you had in mind?

            pthread_rwlock_unlock(&q->active_buffer_rwlock);

            // Check potential race condition with datum_queue_free() before acquiring a lock
            if (q->buffer[buffer_id] == NULL) {
                DLOG_ERROR("Buffer pointer null. Queue already destroyed?");
                return -1;
            }

            // get a write lock for that buffer
            pthread_rwlock_wrlock(&q->buffer_rwlock[buffer_id]);