New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bootloader: use waitpid instead of wait #2966

Merged
merged 1 commit into from Aug 16, 2018

Conversation

Projects
None yet
3 participants
@OneBlue

OneBlue commented Nov 5, 2017

On Linux, wait is used to wait for child process to exit. However, as stated in the manual:

The call wait(&status) is equivalent to:

           waitpid(-1, &status, 0);

Doing so works fine in most cases, but when the child process itself plays with switch_root, it seems to signal the parent process, consequently leaving the function before its child has exited.

It will then delete the previously extracted files, and so create a race condition (the child might not have loaded all its libraries yet).

Using waitpid ensures that the child process that we just created has finished.

@htgoebel htgoebel added the bootloader label Dec 6, 2017

@htgoebel htgoebel added this to the PyInstaller 3.4 milestone Dec 7, 2017

@htgoebel

This comment has been minimized.

Member

htgoebel commented Feb 25, 2018

@codewarrior0 For me this sound reasonable and ready to merge. Can you please state your opinion?

@htgoebel htgoebel requested a review from codewarrior0 Feb 25, 2018

@htgoebel

This comment has been minimized.

Member

htgoebel commented Jul 7, 2018

@codewarrior0 pinging. The change is just one line :-)

@codewarrior0

codewarrior0 approved these changes Jul 8, 2018 edited

No real objections here. The code is more or less equivalent.

I haven't heard of any interaction between waitpid(-1, ...) and switch_root so I don't know exactly how the problem is solved by waiting for a particular PID instead of any PID, so I just have to trust OneBlue that this solution works.

@codewarrior0

This comment has been minimized.

Member

codewarrior0 commented Jul 8, 2018

I'm also a bit curious about the application of calling switch_root from a pyinstaller-built program. Is it some kind of sandboxing?

@OneBlue

This comment has been minimized.

OneBlue commented Jul 8, 2018

@codewarrior0 : Pretty much.
When I discovered this issue, I was a teacher and I needed to run test suites on student code.

I wanted the student code to run in a lightweight sandbox (so a VM would be too heavy) so I basically wrote a python program that would create a chroot, create new network / process namespaces and then call switch_root to jump in to chroot.

@htgoebel

This comment has been minimized.

Member

htgoebel commented Jul 9, 2018

@codewarrior0 Thanks for sharing your thoughts.

@OneBlue Can you please explain in more detail, what is going on when using switch_root with wait vs. waitpid? Esp. why the bootloader parent process gets signaled in your case when using wait.

@OneBlue

This comment has been minimized.

OneBlue commented Jul 9, 2018

I had a deeper look today and I finally understood the root cause.

Here is the output of strace -f (I removed the irrelevant parts to make it more readable):

(/mymoulette is the binary that I built using pyinstaller)

// 1 - switch_root is being executed
7994  execve("/sbin/switch_root", ["/sbin/switch_root", "/mnt", "/mymoulette", "-f", "/bin/bash"], [/* 38 vars */] <unfinished ...>

// 2 - switch_root does whatever it has to do to setup the new virtual filesystem
7994  stat("/mnt", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
7994  stat("/mnt/dev", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
7994  mount("/dev", "/mnt/dev", NULL, MS_MOVE, NULL) = 0
7994  stat("/mnt/proc", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
7994  mount("/proc", "/mnt/proc", NULL, MS_MOVE, NULL) = 0
7994  stat("/mnt/sys", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
7994  mount("/sys", "/mnt/sys", NULL, MS_MOVE, NULL) = 0
7994  stat("/mnt/run", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
7994  mount("/run", "/mnt/run", NULL, MS_MOVE, NULL) = 0
7994  chdir("/mnt")                     = 0
7994  open("/", O_RDONLY)               = 3
7994  mount("/mnt", "/", NULL, MS_MOVE, NULL) = 0
7994  chroot(".")                       = 0

// 3 - Here's the interesting part, switch_root FORKS ! (Note that the child pid is 6)
// (Side note: the pid is that low because this process is running in a new process namespace,
// but that doesn't change anything)
7994  clone(child_stack=NULL,
          flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD,
          child_tidptr=0x7f211db339d0) = 6

// So the pid of the new process is 6 inside that process namespace, but to the root 
// namespace it's 7995

// [redacted] process 7995 does whatever it has to do

// 4 - Process 7994 calls execve on the pyinstaller built binary
7994  execve("/mymoulette", ["/mymoulette", "-f", "/bin/bash"], [/* 38 vars */] <unfinished ...>

// 5 - Process 7995 exits
7995  exit_group(0 <unfinished ...>

// So to sum up, process 7994 (which was originally switch_root, but now is a pyinstaller
// built binary) had a child (7995), which has now exited

// 6 - process 7994 does its bootstrapping stuff

// [redacted] process 7994 extracts librairies 
7994  write(2, "LOADER: Executing self as child\n", 32) = 32
7994  getpid()                          = 5
7994  write(2, "[5] ", 4)               = 4
7994  write(2, "LOADER: set _MEIPASS2 to /tmp/_M"..., 41) = 41
7994  getpid()                          = 5
7994  write(2, "[5] ", 4)               = 4
7994  write(2, "LOADER: LD_LIBRARY_PATH_ORIG=/tm"..., 61) = 61
7994  getpid()                          = 5
7994  write(2, "[5] ", 4)               = 4
7994  write(2, "LOADER: LD_LIBRARY_PATH=/tmp/_ME"..., 72) = 72

// 7 - process 7994 prepares to execute child (note that the new process id is 7 inside
// the process namespace, and 7996 to the root namespace)
7994  clone(child_stack=NULL, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x7f4aa62e19d0) = 7

// [redacted] process 7996 running execve

// 8 - Process 7994 calls wait
7994  wait4(-1, [{WIFEXITED(s) && WEXITSTATUS(s) == 0}], 0, NULL) = 6


// So here's the problem: wait(2) returns 6, and not 7.
// Meaning that we have been notified that process 6 (the one created by switch_root earlier)
// has exited, and not process 7 (the one we just created)

Sorry for long post, I hope it's clear :).

TL;DR: switch_root forks and then calls execve(2) on the binary built with pynstaller, so when pynstaller calls wait(2), it gets notified for the child created by switch_root, not the child it created itself.

@codewarrior0

This comment has been minimized.

Member

codewarrior0 commented Jul 9, 2018

Oh, I see! Any child processes spawned before execve can still be waited on afterward. Kind of a hidden gotcha with wait or waitpid(-1, ... that you have to make sure the child that just died is actually yours.

@htgoebel

This comment has been minimized.

Member

htgoebel commented Aug 16, 2018

Thank you very much for analysing this! This is what I call "bite-sized" - as we say in German.

@htgoebel htgoebel merged commit 355f0c7 into pyinstaller:develop Aug 16, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

mkassner added a commit to pupil-labs/pyinstaller that referenced this pull request Nov 5, 2018

Bootloader: Use waitpid instead of wait.
Using `wait` works fine in most cases, but will also wait for any child
processes spawned before execve. Thus we need to wait for exactly our child.

See pyinstallergh-2966 for a detailed analysis.

cowo78 pushed a commit to cowo78/pyinstaller that referenced this pull request Dec 7, 2018

Blue Giuseppe Corbelli
Bootloader: Use waitpid instead of wait.
Using `wait` works fine in most cases, but will also wait for any child
processes spawned before execve. Thus we need to wait for exactly our child.

See pyinstallergh-2966 for a detailed analysis.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment