-
Notifications
You must be signed in to change notification settings - Fork 34
Fix bugs in datum_queue_free #29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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?
buffer[1] cannot be free'd because its allocated in a single call for buffer[0] and then split. datum_gateway/src/datum_queue.c Line 117 in a384ff6
|
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. |
This fixes the original harmless buglet of zeroing out q->buffer[0] twice Will think about the rest of the potential race condition now.
I'm reading through the code, but I think I found one more thing.
|
Yep, seems so. |
Anything larger than 9999999 doesn't make sense
Is this what you had in mind?
|
This was merged... |
q->buffer[1] doesn't seem to be free()'d
q->buffer[0] is duplicated, you probably meant to zero out buffer[1]
shouldn't the unlocking order be reverse of locking order to prevent deadlocks?