On Wed, Dec 16, 2015 at 02:54:27PM +0100, Mateusz Guzik wrote: > While I agree with analysis the patch does not look right. Since the > struct is only assigned and all locks get dropped, there is nothing > preventing another thread from the forking process to change the process > group. > > Interestngly very same function assigns the pointer explicitely later: > p2->p_pgrp = p1->p_pgrp; > > As such, I would argue the right solution is to move p_pgrp out of the > copied area. Exit path clears the pointer, so it should be fine to just > do that. For reused struct proc it would be enough, but not for the new allocation. Neither init nor ctr for the proc zone do not initialize p_pgrp, so you would end up with the garbage in the pointer. I think that your patch should add explcit zeroing of the member into proc_init(). > > That is (untested): > > diff --git a/sys/sys/proc.h b/sys/sys/proc.h > index 90effa6..cb94318 100644 > --- a/sys/sys/proc.h > +++ b/sys/sys/proc.h > _at__at_ -586,7 +586,6 _at__at_ struct proc { > int p_osrel; /* (x) osreldate for the > binary (from ELF note, if any) */ > char p_comm[MAXCOMLEN + 1]; /* (b) Process name. */ > - struct pgrp *p_pgrp; /* (c + e) Pointer to process group. */ > struct sysentvec *p_sysent; /* (b) Syscall dispatch info. */ > struct pargs *p_args; /* (c) Process arguments. */ > rlim_t p_cpulimit; /* (c) Current CPU limit in seconds. */ > _at__at_ -599,6 +598,7 _at__at_ struct proc { > u_int p_xsig; /* (c) Stop/kill sig. */ > /* End area that is copied on creation. */ > #define p_endcopy p_xsig > + struct pgrp *p_pgrp; /* (c + e) Pointer to process group. */ > struct knlist p_klist; /* (c) Knotes attached to this proc. */ > int p_numthreads; /* (c) Number of threads. */ > struct mdproc p_md; /* Any machine-dependent fields. */ > > -- > Mateusz Guzik <mjguzik gmail.com>Received on Wed Dec 16 2015 - 13:11:43 UTC
This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:41:01 UTC