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
> @@ -586,7 +586,6 @@ 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. */
> @@ -599,6 +598,7 @@ 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>
_______________________________________________
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
