Re: ptrace attach in multi-threaded processes

On Wed, Jul 13, 2016 at 09:42:47AM -0700, Mark Johnston wrote:
> On Wed, Jul 13, 2016 at 07:54:39AM +0300, Konstantin Belousov wrote:
> > I finally see.  Might be something like the patch below is a step in
> > the desired direction.  Idea is in the proc_next_xthread(): p_xthread
> > should be set to the next thread with a pending signal.  Do you have a
> > test case that demonstrates the issue ?
> 
> Not yet, I'll work on one. I've only seen this occur once in an Isilon
> test cluster.
> 
> The diff makes sense to me, thanks. I'd find the code easier to read if
> proc_next_xthread() was a pure function that returned the flagged
> thread instead of setting p_xthread.
I did coded it this way,
struct thread *proc_next_xthread(struct proc *p);
initially, but I did not liked that all uses are
	p->p_xthread = proc_next_xthread(p);

> 
> I'm having trouble determining if the diff changes any userland-visible
> behaviour. It seems that the only potential problem with the current
> p_xthread handling is in stopevent(), since a thread calling stopevent()
> from postsig() may clear p_xthread after it was set by another thread in
> ptracestop(). But I also don't understand why we call stopevent(S_SIG)
> from both issignal() and postsig() - this would appear to stop the
> thread twice for the same signal.
You mean that the patch would not fix your issue ? Quite possible, it
might require some more code to 'move the torch' to next xthread, so to
say. When you write the test case, I will spend efforts on the working
patch.

That said, I do not think that we should change anything about stopevent(),
since this is code which is on life support.  If we cannot remove procfs
debugging interface, let not change it at least in incompatible ways.

> 
> With respect to the desired direction, do you agree that the SIGSTOP
> from PT_ATTACH should effectively be ignored if a different signal stops
> the process first? As I said in a previous post, it seems that the
> SA_STOP property of PT_ATTACH's SIGSTOP is not used in the common case,
> since ptracestop() will stop the process if any signal is received, and
> the PT_DETACH operation will typically overwrite the SIGSTOP with 0 in
> td_xsig.
Hmm, I think no, we can not make such change. Issue is, debugger
interface guarantees (at least for single-threaded programs it is
done correctly) that SIGSTOP is noted. In my opinion, it would be the
incompatible API change.