[C/R]: mktree.c: cr_set_creator() question

[C/R]: mktree.c: cr_set_creator() question

[Sukadev Bhattiprolu]

Oren:

I was trying to run the ptree test I had posted earlier with v14-rc3
and get a SIGSEGV in mktree (when restarting a simple process tree).

The SIGSEGV is due to 'creator' being NULL @ mktree.c:585.

I debugged it a bit and was not clear on the following 'task-ppid == 1'
check. It seems to check for orphaned processes using 'ppid == 1',
but 'ppid == 1' could be 1 if the 'task' was an actual child of the
container-init right ?

Also, 'phantom' does not seem to be set anywhere and so 'creator'
ends up being NULL.

Since in my case, 'task' refers to an actual child of container-init,
I reversed the order of the following two checks as a quick hack, and
it seems to fix the SIGSEGV. 

[v14-rc3] mktree.c: cr_set_creator() (lines 543-553)

        } else if (task->ppid == 1) {
                /* (non-session-leader) orphan: creator is dummy */
                cr_dbg("pid %d: orphan session %d\n", task->pid, task->sid);
                if (!session->phantom)
                        if (cr_placeholder_task(ctx, task) < 0)
                                return -1;
                creator = session->phantom;
        } else if (task->sid == parent->sid) {
                /* (non-session-leader) inherit: creator is parent */
                cr_dbg("pid %d: inherit sid %d\n", task->pid, task->sid);
                creator = parent;


Sukadev

Re: [C/R]: mktree.c: cr_set_creator() question

[Oren Laadan]

On Wed, 8 Apr 2009, Sukadev Bhattiprolu wrote:

Indeed the bug was that cr_placeholder_task() forgot to also set
'session->phantom = holder'. This fixes the crash (added to next
version).

diff --git a/mktree.c b/mktree.c
index 08cd56a..2e0705d 100644
--- a/mktree.c
+++ b/mktree.c
@@ -540,6 +540,10 @@ static int cr_set_creator(struct cr_ctx *ctx, struct task *task)
 		/* dead: creator is session leader */
 		cr_dbg("pid %d: task is dead\n", task->pid);
 		creator = session;
+	} else if (task->sid == parent->sid) {
+		/* (non-session-leader) inherit: creator is parent */
+		cr_dbg("pid %d: inherit sid %d\n", task->pid, task->sid);
+		creator = parent;
 	} else if (task->ppid == 1) {
 		/* (non-session-leader) orphan: creator is dummy */
 		cr_dbg("pid %d: orphan session %d\n", task->pid, task->sid);
@@ -547,10 +551,6 @@ static int cr_set_creator(struct cr_ctx *ctx, struct task *task)
 			if (cr_placeholder_task(ctx, task) < 0)
 				return -1;
 		creator = session->phantom;
-	} else if (task->sid == parent->sid) {
-		/* (non-session-leader) inherit: creator is parent */
-		cr_dbg("pid %d: inherit sid %d\n", task->pid, task->sid);
-		creator = parent;
 	} else {
 		/* first make sure we know the session's creator */
 		if (!session->creator && session != cr_init_task(ctx)) {
@@ -643,6 +643,7 @@ static int cr_placeholder_task(struct cr_ctx *ctx, struct task *task)
 		session->children->prev_sib = holder;
 	}
 	session->children = holder;
+	session->phantom = holder;
 
 	/* reparent entry if necssary */
 	if (task->next_sib)


> Oren:
> 
> I was trying to run the ptree test I had posted earlier with v14-rc3
> and get a SIGSEGV in mktree (when restarting a simple process tree).
> 
> The SIGSEGV is due to 'creator' being NULL @ mktree.c:585.
> 
> I debugged it a bit and was not clear on the following 'task-ppid == 1'
> check. It seems to check for orphaned processes using 'ppid == 1',
> but 'ppid == 1' could be 1 if the 'task' was an actual child of the
> container-init right ?
> 

Yes, a process with ppid==1 and session id like the container-init 
can be viewed either an orphan task or as a child of containter-init.

> Also, 'phantom' does not seem to be set anywhere and so 'creator'
> ends up being NULL.

This was the bug :(

> 
> Since in my case, 'task' refers to an actual child of container-init,
> I reversed the order of the following two checks as a quick hack, and
> it seems to fix the SIGSEGV. 

It fixed the crash because it was a child of the session leader. If
wouldn't work for an orphan with a different session id... (the fix
above does).

You are definitely right in observing that a "phantom" task is not
needed (although it doesn't break either) if an orphan has the same 
session id as the container root.

Oren.

> 
> [v14-rc3] mktree.c: cr_set_creator() (lines 543-553)
> 
>         } else if (task->ppid == 1) {
>                 /* (non-session-leader) orphan: creator is dummy */
>                 cr_dbg("pid %d: orphan session %d\n", task->pid, task->sid);
>                 if (!session->phantom)
>                         if (cr_placeholder_task(ctx, task) < 0)
>                                 return -1;
>                 creator = session->phantom;
>         } else if (task->sid == parent->sid) {
>                 /* (non-session-leader) inherit: creator is parent */
>                 cr_dbg("pid %d: inherit sid %d\n", task->pid, task->sid);
>                 creator = parent;
> 
> 
> Sukadev
> 
>