Skip to content

Preload death#1504

Merged
sporksmith merged 4 commits intoshadow:mainfrom
sporksmith:preload-death
Jul 12, 2021
Merged

Preload death#1504
sporksmith merged 4 commits intoshadow:mainfrom
sporksmith:preload-death

Conversation

@sporksmith
Copy link
Copy Markdown
Contributor

@sporksmith sporksmith commented Jul 8, 2021 •
edited
Loading

  • Adds a rust utility class ChildPidWatcher, which uses pidfd and epoll to detect child process death and notify via registered callbacks.
  • In IpcData, add an API for notifying that the plugin has exited.
  • In thread_preload, register a callback with ChildPidWatcher that uses the new IpcData API.
  • Remove shim-side global destructor, so that shadow continues to
    capture all syscalls until the process actually exits.

Fixes #1476

@github-actions github-actions bot added Component: Build Build/install tools and dependencies Component: Libraries Support functions like LD_PRELOAD and logging Component: Main Composing the core Shadow executable Component: Testing Unit and integration tests and frameworks labels Jul 8, 2021
@sporksmith sporksmith force-pushed the preload-death branch 2 times, most recently from 7194462 to 06a1daf Compare July 8, 2021 21:55
@sporksmith sporksmith requested a review from stevenengler July 8, 2021 21:55
@codecov
Copy link
Copy Markdown

codecov bot commented Jul 8, 2021 •
edited
Loading

Codecov Report

Merging #1504 (5c12771) into main (4730119) will decrease coverage by 0.09%.
The diff coverage is 6.25%.

❗ Current head 5c12771 differs from pull request most recent head 022d3bf. Consider uploading reports for the commit 022d3bf to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1504      +/-   ##
==========================================
- Coverage   53.00%   52.91%   -0.10%     
==========================================
  Files         140      140              
  Lines       21004    21035      +31     
  Branches     5318     5321       +3     
==========================================
- Hits        11134    11131       -3     
- Misses       6941     6970      +29     
- Partials     2929     2934       +5
Flag Coverage Δ
tests 52.91% <6.25%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/lib/logger/rust_bindings/src/bindings.rs 0.00% <ø> (ø)
src/lib/shim/binary_spinning_sem.cc 0.00% <0.00%> (ø)
src/lib/shim/ipc.cc 0.00% <0.00%> (ø)
src/lib/shim/shim.c 30.63% <ø> (+0.36%) ⬆️
src/main/core/worker.c 74.65% <0.00%> (-0.52%) ⬇️
src/main/host/process.c 69.67% <ø> (ø)
src/main/host/thread_preload.c 0.00% <0.00%> (ø)
src/test/determinism/test_determinism.c 60.00% <ø> (-0.34%) ⬇️
src/main/core/manager.c 70.90% <60.00%> (-0.17%) ⬇️
src/main/host/descriptor/epoll.c 77.73% <0.00%> (-0.71%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4730119...022d3bf. Read the comment docs.

@sporksmith sporksmith removed the request for review from stevenengler July 8, 2021 22:30
@sporksmith
Copy link
Copy Markdown
Contributor Author

sporksmith commented Jul 8, 2021

Oops, I seem to have jumped the gun on tests passing, and accidentally set the reviewer to @stevenengler instead of Ryan. Please disregard, and I'll see if I can sort out the remaining issues...

@sporksmith sporksmith force-pushed the preload-death branch 2 times, most recently from 4cd3b9c to f3874cd Compare July 10, 2021 01:49
@sporksmith sporksmith requested a review from stevenengler July 10, 2021 02:02
@sporksmith
Copy link
Copy Markdown
Contributor Author

sporksmith commented Jul 10, 2021

Sending to @stevenengler after all since it's more outside of IPC and preload than in it, though @rwails you're welcome to take a look too if you have cycles :)

stevenengler
stevenengler approved these changes Jul 12, 2021
View reviewed changes
@rwails
Copy link
Copy Markdown
Collaborator

rwails commented Jul 12, 2021

p.s. I can take a look at this late tonight if you want my review, but if you're ready to merge earlier than that, go for it!

@sporksmith
Use SIGCHLD to detect when preload-managed processes exit
3ecd2a5 
* Register managed process PIDs in a global pid -> IPC map.
* Install a SIGCHLD handler that, on process death, marks
  all of its threads dead and wakes up any Shadow threads
  waiting on IPC.
* Remove shim-side global destructor, so that shadow continues to
  capture all syscalls until the process actually exits.

Fixes shadow#1476
@sporksmith
Remove old workaround and comments about shadow#1476
e89f24a
@sporksmith
childpid watcher, using waitpid
c79f2f6 
Adds ChildPidWatcher, which uses a dedicated thread to `wait` on all
children and notify via callbacks.

This turns out to be incompatible with ptrace; I didn't think `wait`
called from a non-tracer thread would get the ptrace stops, but it looks
like it does.
@sporksmith sporksmith enabled auto-merge July 12, 2021 19:18
@sporksmith
Change ChildPidWatcher to use pidfd and epoll
022d3bf 
We use `pidfd_open` to get a file descriptor for each child pid, which
we can then monitor with `epoll`. This doesn't interact with other
process state transitions (such as ptrace stops).
@sporksmith sporksmith merged commit cc38754 into shadow:main Jul 12, 2021
@sporksmith sporksmith deleted the preload-death branch July 12, 2021 20:02
@sporksmith sporksmith mentioned this pull request Mar 2, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stevenengler stevenengler stevenengler approved these changes

Assignees

@sporksmith sporksmith

Labels

Component: Build Build/install tools and dependencies Component: Libraries Support functions like LD_PRELOAD and logging Component: Main Composing the core Shadow executable Component: Testing Unit and integration tests and frameworks

Projects

None yet

Milestone

Fully support process-based simulatio...

Development

Successfully merging this pull request may close these issues.

thread_preload doesn't interpose some exit handlers, doesn't catch abnormal process exit

4 participants

@sporksmith @rwails @stevenengler @robgjansen