Skip to content

Conversation

@teqwve
Copy link
Contributor

@teqwve teqwve commented Jul 22, 2019

This PR will (hopefully in near future) add support for threads

@teqwve teqwve changed the title WIP Add threads support WIP Support threads Jul 22, 2019
@teqwve teqwve force-pushed the threads branch 4 times, most recently from 715c9f6 to 7151362 Compare July 22, 2019 22:54
@teqwve
Copy link
Contributor Author

teqwve commented Jul 22, 2019

Tested with simple program:

#include <iostream>
#include <thread>

int main() {
  std::cout << "In parent, before clone" << std::endl;

  std::thread th{
    []() {
      std::cout << "In child" << std::endl;
    }
  };

  std::cout << "In parent, after clone" << std::endl;
  th.join();
}

which is compiled and executed as:

teqwve sio2jail/build $ g++ -std=c++17 -static -Wl,--whole-archive -lpthread -Wl,--no-whole-archive test.cc
teqwve sio2jail/build $ ./src/sio2jail -p permissive -b ./minimal:/:ro -- ./a.out
In parent, before clone
In parent, after clone
In child
__RESULT__ 124 0 0 141804 0
memory limit exceeded

(btw, quite funny, seems like we don't set memory limit (as expected with -m 0), but test it in output builder).

There is a problem when executing this with a memory limit (detailed comment can be found in code in this PR in MemoryLimitListener.cc):

teqwve sio2jail/build $ ./src/sio2jail -l log.txt -s -p permissive -b ./minimal:/:ro -m 1G -- ./a.out
In parent, before clone
terminate called after throwing an instance of 'std::system_error'
  what():  Resource temporarily unavailable
__RESULT__ 6 8 0 2684 0
process exited due to signal 6

In log.txt there is:

2019-07-23 01:00:07.928089      DEBUG   Detected syscall architecture x86_64 from traceEventMsg=18 for mmap(9) (0, 1082134528, 0, 131106, 4294967295, 0)

And we have that:

  • 131106 contains MAP_STACK flag.
  • 1082134528 = 1024 * 1024 * 1024 = 1G (we set the RLIMIT_AS and RLIMIT_STACK to the same value, and to this value)

So it looks like it should be enough to add extra margin to RLIMIT_AS (like 2 times real limit?). I didn't have enough time to investigate it today.

@teqwve
Copy link
Contributor Author

teqwve commented Jul 22, 2019

Yeah, it definitely should help, look for getrlimit in pthread source code :)

@teqwve
Copy link
Contributor Author

teqwve commented Jul 23, 2019

Fixed by setting stack limit to infinity (address space is still limited so...).

@teqwve teqwve force-pushed the threads branch 7 times, most recently from a1e0b2f to 584b007 Compare July 23, 2019 22:00
@teqwve
Copy link
Contributor Author

teqwve commented Jul 23, 2019

Looks better, I've tested memory limit with:

#include <iostream>
#include <thread>

int main() {
    std::thread th{[]() {
        int res = 0;

        long unsigned int used = 0;
        do {
            used += 1024;

            int* tmp = new int[1024 / sizeof(int)];
            if (tmp == NULL)
                break;

            res += tmp[1024 / sizeof(int) - 1];
        } while (used < 2ULL * 1024 * 1024 * 1024);

        printf("FAIL used %lumb\n", used / 1024 / 1024, res);
    }};
    th.join();
}

compiled with:

teqwve sio2jail/build $ g++ -std=c++14 -static -Wl,--whole-archive -lpthread -Wl,--no-whole-archive test.cc

and executed as:

teqwve sio2jail/build $ ./src/sio2jail -b ./minimal:/:ro -m 1G -- ./a.out
__RESULT__ 124 0 0 1118704 0
memory limit exceeded
teqwve sio2jail/build $ ./src/sio2jail -b ./minimal:/:ro -m 3G -- ./a.out
FAIL used 2048mb
__RESULT__ 0 0 0 2167280 0
ok

In both cases tested with modified default policy (running under permissive caused every small mmap to be traced which is too expensive):

diff --git a/src/seccomp/policy/DefaultPolicy.cc b/src/seccomp/policy/DefaultPolicy.cc
index 59d3498..31a0198 100644
--- a/src/seccomp/policy/DefaultPolicy.cc
+++ b/src/seccomp/policy/DefaultPolicy.cc
@@ -41,6 +41,8 @@ void DefaultPolicy::addExecutionControlRules(bool allowFork) {
                    "sigaltstack",
                    "sigsuspend"});
 
+    allowSyscalls({"clone", "madvise"});
+
     rules_.emplace_back(SeccompRule(
             "set_thread_area", action::ActionTrace([](auto& /* tracee */) {
                 // Allow syscall, let sio2jail detect syscall architecture

teqwve added 2 commits July 27, 2019 09:22
Make code more general - SeccompListener and TraceExecutor don't need
assumption that there is only process supervised.
@teqwve teqwve force-pushed the threads branch 2 times, most recently from d011605 to 29481dc Compare July 28, 2019 10:05
@teqwve teqwve marked this pull request as ready for review July 28, 2019 10:17
@teqwve teqwve changed the title WIP Support threads Support threads Jul 28, 2019
@teqwve teqwve requested a review from Wolf480pl July 28, 2019 10:17
@teqwve
Copy link
Contributor Author

teqwve commented Jul 28, 2019

@Wolf480pl Could you look at this for a second? Look likes it' read for a review (i'll add some memory and threads limit tests, but I won't change any sio2jail code anymore").

I've a bad feeling about how address space usage increases with threads (stack is mmaped and we count it). With 1 thread we have 135mb, with 10 threads 743mb.

teqwve@mavo ~/data/projects/sio2jail/build $ ./src/sio2jail -b ./boxes/minimal:/:ro -t 10 -m 1G -- ./test/src/1-sec-prog-th deep 1
0
__RESULT__ 0 1000 0 135664 0
ok
teqwve@mavo ~/data/projects/sio2jail/build $ ./src/sio2jail -b ./boxes/minimal:/:ro -t 10 -m 1G -- ./test/src/1-sec-prog-th deep 10
-428549568
-428549568
-428549568
-428549568
-428549568
-428549568
-428549568
-428549568
-428549568
-428549568
__RESULT__ 0 1000 0 743956 0
ok

Copy link
Member

@Wolf480pl Wolf480pl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A gdzie 1-sec-prog-th.c ?

@teqwve teqwve force-pushed the threads branch 2 times, most recently from 3a832fa to 347f06f Compare July 28, 2019 23:13
Copy link
Member

@Wolf480pl Wolf480pl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jeszcze to ESRCH mnie martwi, a poza tym ok.

As it's quite common situtation it should not be handled using
exceptions. Morever, just in case, we want all steps to execute (even in
case previous steps threw an exception).
@teqwve teqwve requested a review from Wolf480pl July 30, 2019 20:06
Copy link
Member

@Wolf480pl Wolf480pl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

@teqwve teqwve merged commit 8b7dd18 into master Jul 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants