Possible deadlock resulting from child receiving signal between TraceExecutor and PerfListener

[MikolajKolek]

After the parent finishes TraceExecutor::onPostForkParent, every time the child receives a signal, it stops, waiting for for a PTRACE_CONT. This not a problem when PerfListener isn't enabled, as when the parent begins its waitid loop in Executor, it will receive the ptrace notification, and the child will be resumed by TraceExecutor::onExecuteEvent.

However, if PerfListener is enabled, it executes after TraceExecutor. It contains a pthread barrier, which requires the presence of both the parent and the child. If the child receives a signal before either of them reach the barrier, the program will be deadlocked, with the child never receiving the PTRACE_CONT, and the parent never moving past the barrier.

Keep in mind this problem is probably mostly theoretical, as it would require:

• A non-critical signal to be delivered to the child at all in that timespan, which is almost impossible with normal execution (however can happen, for example with SIGWINCH)
• The signal needs to hit an extremely precise point in time between TraceExecutor and PerfListener

With that said, I've managed to reproduce it by adding a sleep(1) at the end of TraceExecutor::onPostForkParent and using that time to resize the window and cause a SIGWINCH to be sent to the child, deadlocking the process. I believe this issue can probably be fixed by moving TraceExecutor after PerfListener in the listener order, however I don't know if that won't break anything else.

A similar problem might also theoretically exist in the TraceExecutor, this time not requiring PerfListener. TraceExecutor::onPostForkParent does only one waitpid call, assuming it will receive the exact SIGTRAP sent by the child in TraceExecutor::onPostForkChild. This might theoretically not be correct if it receives a SIGTSTP before the SIGTRAP or finishes with EINTR, but that seems even more unlikely than the previous scenario to me.

[Wolf480pl]

How did you find this?
Did you actually stumble upon a deadlock in the wild, and traced it down to this barrier?
Or did you just find it while reading code?

Anyway, there doesn't seem like there's anything in PerfListener that depends on TraceExectutor, so it technically should be possible to move it before. I'm trying to think of what side effects that'd have...

But since PerfListener::onPostForkParent initially disables the events and uses enable_on_exec

sio2jail/src/perf/PerfListener.cc

Lines 187 to 188 in deed83e

	attrs.disabled = 1;
	attrs.enable_on_exec = 1;

then it shouldn't matter when perf_event_open is called, or at least it shouldn't affect the measurement results.

IIRC the barrier is only there so that the child does not get past the exec before perf_event_open is done.
But in that case there's no need for it to stop the parent, it seems... so it could just be a semaphore or a condition variable, or even sth like Linux kernel's completion (unfortunately pthreads doesn't have this one :/ )

Although, it seems like in some cases that barrier was accidentally preventing a desynced TraceExecutor from continuing,
i.e. it could be limiting the consequences of the second more unlikely race condition you described. where the initial waitid in TraceExecutor::onPostForkParent is not the SIGTRAP from TraceExecutor::onPostForkChild...

So is there a scenario in which:

• the waitids in TraceExecutor get mismatched with the events they're supposed to receive from the child
• if PerfListener is after TraceExecutor, it will prevent the child from reaching exec
• if PerfListener is disabled or before TraceExecutor, the child will be able to reach exec
  and how messed up will TraceExecutor be if that happens?

If there is no such scenario, then we have two options to fix this:

• move PerfListener before TraceExecutor
• instead of a barrier, use another synchronization primitive which only blocks the child, not the parent

If there is such scenario that breaks TraceExecutor, then we should also think how we could prevent the waitids from desyncing.