Copilot commented on code in PR #13170:
URL: https://github.com/apache/trafficserver/pull/13170#discussion_r3405658872
##########
src/tscore/ink_queue.cc:
##########
@@ -517,76 +608,96 @@ ink_freelists_dump(FILE *f)
void
ink_atomiclist_init(InkAtomicList *l, const char *name, uint32_t
offset_to_next)
{
+ // The pointers we push onto the atomiclist will also need to be aligned. If
+ // the offset is not aligned, then it is not possible for the caller to
+ // determine a consistent, safe alignment for the object the void* objects
+ // are subobjects of.
+ ink_release_assert(offset_to_next % alignof(void *) == 0);
+
l->name = name;
l->offset = offset_to_next;
- SET_FREELIST_POINTER_VERSION(l->head, FROM_PTR(0), 0);
+ head_p empty_head;
+ SET_FREELIST_POINTER_VERSION(empty_head, FROM_PTR(nullptr), 0);
+ l->head.store(empty_head);
}
void *
ink_atomiclist_pop(InkAtomicList *l)
{
head_p item;
head_p next;
- int result = 0;
+ bool result = 0;
+
+ std::lock_guard guard{l->m};
+
+ item = l->head.load();
do {
- INK_QUEUE_LD(item, l->head);
if (TO_PTR(FREELIST_POINTER(item)) == nullptr) {
return nullptr;
}
- SET_FREELIST_POINTER_VERSION(next,
*ADDRESS_OF_NEXT(TO_PTR(FREELIST_POINTER(item)), l->offset),
FREELIST_VERSION(item) + 1);
- result = ink_atomic_cas(&l->head.data, item.data, next.data);
- } while (result == 0);
- {
- void *ret = TO_PTR(FREELIST_POINTER(item));
- *ADDRESS_OF_NEXT(ret, l->offset) = nullptr;
- return ret;
- }
+ void **next_ptr = to_voidp_p(reinterpret_cast<unsigned char
*>(TO_PTR(FREELIST_POINTER(item))), l->offset);
+ SET_FREELIST_POINTER_VERSION(next, *next_ptr, FREELIST_VERSION(item) + 1);
+ result = l->head.compare_exchange_weak(item, next);
+ } while (result == false);
+
+ void *ret = TO_PTR(FREELIST_POINTER(item));
+ void **ret_ = to_voidp_p(reinterpret_cast<unsigned char *>(ret), l->offset);
+ *ret_ = nullptr;
+ return ret;
}
void *
ink_atomiclist_popall(InkAtomicList *l)
{
head_p item;
head_p next;
- int result = 0;
+ bool result = false;
+
+ item = l->head.load();
Review Comment:
`ink_atomiclist_pop()` is now protected by `l->m` to avoid UB from reading a
node’s `next` field while another thread can overwrite it, but
`ink_atomiclist_popall()` still runs without that mutex and rewrites every
node’s `next` field in its fixup loop. This can race with a concurrent
`ink_atomiclist_pop()` that has already loaded the old head and is about to
read that head node’s `next` pointer (read vs write on the same memory),
reintroducing a data race/UB.
To keep the mutual-exclusion guarantee consistent, `ink_atomiclist_popall()`
should take the same mutex (or otherwise coordinate with `pop()` so it cannot
modify detached nodes while a `pop()` is still reading them).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
