[PATCH] Fix multifarious bad pinfo accesses [was Re: Expect goes crazy... spinning cpu in kill_pgrp]

Dave Korn dave.korn.cygwin@googlemail.com
Sat Jan 9 20:22:00 GMT 2010

Christopher Faylor wrote:

> These abbreviated records are not supposed to be accessed for things
> like "ctty" since it could be irrelevant.  If something is attempting to
> access the ctty then it is wrong and that's what needs to be fixed.
> One of the reasons for keeping an abbreviated structure is to catch
> situations like this, in fact.

  :) Well, it sure works for that.... 100% cpu tends to get my attention!

> True, but you still have a problem if you're accessing a pinfo structure
> which is referencing a soon-to-be-execed process.  You could be sending
> signals to it or attempting to otherwise manipulate it.

  Yeh.  *If* I understand what's happening here, we're tracking down all the
processes in a pgrp by the fact that they all live under the same master tty
in order to kill them.  I'm not really au fait with the unix tty stuff, but I
guess it's possible the child could have detached and the value cached in the
stub would be stale, relative to the changed value in the full pinfo in the
real child process?

> But you'd still be accessing ctty incorrectly.

  Would the /really/ correct thing in kill_pgrp be to ignore this pinfo and
just let it carry on looping, processing real pinfos only?

> I'd prefer something like this:
> --- pinfo.cc    18 Dec 2009 20:32:04 -0000      1.258
> +++ pinfo.cc    9 Jan 2010 16:35:47 -0000
> @@ -416,7 +416,7 @@
>  bool __stdcall
>  _pinfo::exists ()
>  {
> -  return this && !(process_state & PID_EXITED);
> +  return this && !(process_state & (PID_EXITED | PID_EXECED));
>  }
>  bool
> That says that an execed "pinfo" doesn't really exist if it has been
> execed.  If that causes a few problems to be fixed up, that's ok.  I'd
> rather fix those than lie about the ctty of a nonexistent process.

  Yeah, but it's got a TOCTOU race doesn't it?  We'd need to wrap a lock or
critical section around every call to exists() and the subsequent accesses to
_pinfo members.  If not, how come?


More information about the Cygwin-patches mailing list