Coverage increased (+0.006%) to 80.525% when pulling edaeb93 on akonradi:remove-evsig-base-fd into 246f440 on libevent:master.

In the scenario you present, it's certainly possible for evsig_handler() to send the operation to the "wrong" base in terms of absolute order, but that's always going to be a possibility since evsig_handler() can't grab a lock. My goal is to make the resulting trace linearizable, where the resulting linear trace would have evsig_handler() run before evsig_add(). Since evsig_handler() now keeps a local copy of evsig_base, its view is of the state before evsig_add() is called. This prevents a write from getting lost if, in your trace above, evsig_base_fd was -1 but evsig_base was updated to a non-null value.

Furthermore, it's worth noting that in the case you describe (without this patch), it's possible for either the compiler or processor to reorder the writes to evsig_base and evsig_base_fd such that either one is written first, which could result in the fd that receives the write being the one from the new base before evsig_base is set. This patch eliminates that possibility (assuming pointer writes are atomic).

I'd be happy to change the patch description but I think there is a real bug here that I intended this patch to address.

By "make the resulting trace linearizable", I mean that for any trace that executes across multiple processors, there exists some ordering on a single processor such that the result of executing the single-processor and multi-processor traces is the same. Taking your original trace but with this patch applied, we'd have

T1                              T2

evsignal_add()                  evsig_handler()
evsig_add()
EVSIGBASE_LOCK()
                                base = evsig_base
evsig_base = base
                                base_fd = base->sig.ev_signal_pair[1];
                                write(base_fd, &sig, 1, 0)

so while the write to base_fd is not using the evsig_base set by T1, the effect is the same as if the following trace was run on a single core:

evsignal_add()
evsig_handler()
evsig_add()

You're right that this is still subject to use-after-free issues, unlikely as they might be. I don't think it's completely possible to solve that while the caller is the one allocating the memory used in evsig_base.

Since evsig_handler() now keeps a local copy of evsig_base

It just holds a pointer to the base, not the base itself.

Right, that was what I meant. It holds a copy of evsig_base, though, so if evsig_base changes during execution, it still sees the original value.

write(base.fd) <-- closed fd and use-after-tree (UAF), and UAF can SIGSEGV the program while EBADF not, although it is till very bad thing

This will not cause a segfault because base_fd is a local variable allocated on the signal handler's stack. The potential segfault would occur when base_fd is initialized, though as you mention above, it would require a particularly tricky race sequence and I don't think is easily solvable, and definitely out of scope of this PR.

I don't think this answers everything you brought up, but hopefully it clears some things up?

write(base.fd) # SIGSEGV

This will not cause a segfault because base_fd is a local variable allocated on the signal handler's stack

By base.fd I meant base->sig.ev_signal_pair[1]

I don't think is easily solvable, and definitely out of scope of this PR.

SIGSEGV is not possible with the code that we have now (even though it sucks in a lot of other ways).

And speaking about this problem, have you triggered the race that you report with your code? Or it is just about possibility?

And speaking about this problem, have you triggered the race that you report with your code? Or it is just about possibility?

I haven't seen the race occur in practice, but I've seen TSAN errors reported for code using libevent around evsig_base_fd that don't reproduce with this patch.

@azat do you have suggestions on how to move this PR forward? We're likely to hit this in OSS Envoy/libevent now that we're enabling TSAN runs (see a precursor TSAN race at envoyproxy/envoy#6083).