Eek. I don't like seeing refcounted objects that are accessible when their refcount is zero. That's a violation of an important invariant. This is really hard to think through. I'm not sure that my sequence of events here is right either, but I don't think that this change addresses the core problem. Looking at `NSC_CloseSession`, say that two threads attempt to call that `NSC_CloseSession` at the same time. Both could pull a valid pointer from sftk_SessionFromHandle if the two calls happen at around same time. The lock is released several times in this process. I think that the lock is consistent in covering both the `refCount` and the reference to the session from the slot, but the interaction of the two could be a problem. Assuming that refCount == 1 to start, this looks like a problematic sequence of events. ``` A: sftk_SessionFromHandle => refCount++ B: sftk_SessionFromHandle => refCount++ A: sftkqueue_delete and refCount-- B: skip the delete block because the session is no longer attached to the slot B: sftk_FreeSession => refCount-- B: refCount was reduced to 1, call free() A: sftk_FreeSession => UAF on sftk_SlotFromSession as it attempts to read session->slot ``` All of those steps, aside from the last two are covered by the lock. The scary part is that covering the free() with the lock wouldn't help. The point is that A is holding a reference to an object that is now dead. The problem is that there is an interaction between reference counting and the ownership here. Given that this object is pinned to the slot and only freed on shutdown, I have a more dramatic simplification to suggest: remove the reference count entirely. Then use the lock to govern its insertion and removal to and from the slot. Only delete the object if it was removed from the slot. That doesn't address the problem of sessions that are in active use during shutdown. Notice that `NSC_CloseSession` doesn't assert that the reference count is 1, it asserts that it is greater than zero, which suggests that this code might not be careful enough about tracking outstanding uses before hitting the shutdown code. Changing that assertion to be a ==1 and running tests would be illuminating. Other than that check, I would advocate for dumping the reference count entirely.
Bug 1508776 Comment 17 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
Eek. I don't like seeing refcounted objects that are accessible when their refcount is zero. That's a violation of an important invariant. This is really hard to think through. I'm not sure that my sequence of events here is right either, but I don't think that this change addresses the core problem. Looking at `NSC_CloseSession`, say that two threads attempt to call that `NSC_CloseSession` at the same time. Both could pull a valid pointer from sftk_SessionFromHandle if the two calls happen at around same time. The lock is released several times in this process. I think that the lock is consistent in covering both the `refCount` and the reference to the session from the slot, but the interaction of the two could be a problem. Assuming that refCount == 1 to start, this looks like a problematic sequence of events. The order of the first two steps doesn't matter. ``` A: sftk_SessionFromHandle => refCount++ B: sftk_SessionFromHandle => refCount++ A: sftkqueue_delete and refCount-- B: skip the delete block because the session is no longer attached to the slot B: sftk_FreeSession => refCount-- B: refCount was reduced to 1, call free() A: sftk_FreeSession => UAF on sftk_SlotFromSession as it attempts to read session->slot ``` All of those steps, aside from the last two, are independently covered by the lock. The scary part is that covering the free() with the lock wouldn't help. The point is that A is holding a reference to an object that has been freed. The problem is that there is an interaction between reference counting and the ownership here. Given that this object is pinned to the slot and only freed on shutdown, I have a more dramatic simplification to suggest: remove the reference count entirely. Then use the lock to govern its insertion and removal to and from the slot. Only delete the object if it was removed from the slot. That doesn't address the problem of sessions that are in active use during shutdown. Notice that `NSC_CloseSession` doesn't assert that the reference count is 1, it asserts that it is greater than zero, which suggests that this code might not be careful enough about tracking outstanding uses before hitting the shutdown code. Changing that assertion to be a ==1 and running tests would be illuminating. Other than that check, I would advocate for dumping the reference count entirely.