Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

Commit

Permalink
[LibOS] Correctly propagate exit code from threads
Browse files Browse the repository at this point in the history
Currently, a thread calling exit_group() will cause all other
threads to also exit, but the exit code for these threads will be
0. As a result, another process calling wait() might receive 0
instead of the right exit code.

Instead, make sure that all other threads exit with the same
exit_code and term_signal.

It looks like this regression was introduced in
50ca6c4. I noticed it because
LTP test accept02 incorrectly returned exit code 0 before failing
(see #1937).

The previous commit strengthens a test (exit_group) that triggers
the described scenario.
  • Loading branch information
pwmarcz committed Nov 4, 2020
1 parent a5ebcb2 commit de6974a
Show file tree
Hide file tree
Showing 5 changed files with 30 additions and 8 deletions.
3 changes: 2 additions & 1 deletion LibOS/shim/include/shim_thread.h
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ struct shim_thread {
int term_signal; // Store the terminating signal, if any; needed for
// wait() and friends
bool is_alive;
/* Trigger thread exit. Set together with exit_code and term_signal. */
bool time_to_die;

PAL_HANDLE child_exit_event;
Expand Down Expand Up @@ -299,7 +300,7 @@ static inline void set_handle_map(struct shim_thread* thread, struct shim_handle
}

int thread_destroy(struct shim_thread* self, bool send_ipc);
bool kill_other_threads(void);
bool kill_other_threads(int error_code, int term_signal);
noreturn void thread_exit(int error_code, int term_signal);
noreturn void process_exit(int error_code, int term_signal);

Expand Down
3 changes: 2 additions & 1 deletion LibOS/shim/src/bookkeep/shim_signal.c
Original file line number Diff line number Diff line change
Expand Up @@ -762,7 +762,8 @@ void __handle_signals(shim_tcb_t* tcb) {
}

if (thread->time_to_die) {
thread_exit(/*error_code=*/0, /*term_signal=*/0);
debug("thread exit triggered from another thread\n");
thread_exit(-thread->exit_code, thread->term_signal);
}

while (__atomic_load_n(&thread->pending_signals, __ATOMIC_ACQUIRE)
Expand Down
2 changes: 1 addition & 1 deletion LibOS/shim/src/sys/shim_exec.c
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ int shim_do_execve(const char* file, const char** argv, const char** envp) {
/* Just exit current thread. */
thread_exit(/*error_code=*/0, /*term_signal=*/0);
}
bool threads_killed = kill_other_threads();
bool threads_killed = kill_other_threads(/*error_code=*/0, /*term_signal=*/0);

/* All other threads are dead. Restoring initial value in case we stay inside same process
* instance and call execve again. */
Expand Down
26 changes: 21 additions & 5 deletions LibOS/shim/src/sys/shim_exit.c
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@
#include "shim_thread.h"
#include "shim_utils.h"

struct thread_exit_params {
struct shim_thread* thread;
int exit_code;
int term_signal;
};

int thread_destroy(struct shim_thread* thread, bool send_ipc) {
bool sent_exit_msg = false;

Expand Down Expand Up @@ -150,7 +156,9 @@ noreturn void thread_exit(int error_code, int term_signal) {
}

static int mark_thread_to_die(struct shim_thread* thread, void* arg) {
if (thread == (struct shim_thread*)arg) {
struct thread_exit_params* params = arg;

if (thread == params->thread) {
return 0;
}

Expand All @@ -161,6 +169,8 @@ static int mark_thread_to_die(struct shim_thread* thread, void* arg) {
need_wakeup = true;
}
thread->time_to_die = true;
thread->exit_code = params->exit_code;
thread->term_signal = params->term_signal;
unlock(&thread->lock);

/* Now let's kick `thread`, so that it notices (in `__handle_signals`) the flag `time_to_die`
Expand All @@ -172,11 +182,17 @@ static int mark_thread_to_die(struct shim_thread* thread, void* arg) {
return 1;
}

bool kill_other_threads(void) {
bool kill_other_threads(int error_code, int term_signal) {
struct thread_exit_params params = {
.thread = get_cur_thread(),
.exit_code = -error_code,
.term_signal = term_signal,
};

bool killed = false;
/* Tell other threads to exit. Since `mark_thread_to_die` never returns an error, this call
* cannot fail. */
if (walk_thread_list(mark_thread_to_die, get_cur_thread(), /*one_shot=*/false) != -ESRCH) {
if (walk_thread_list(mark_thread_to_die, &params, /*one_shot=*/false) != -ESRCH) {
killed = true;
}
DkThreadYieldExecution();
Expand All @@ -185,7 +201,7 @@ bool kill_other_threads(void) {
while (!check_last_thread()) {
/* Tell other threads to exit again - the previous announcement could have been missed by
* threads that were just being created. */
if (walk_thread_list(mark_thread_to_die, get_cur_thread(), /*one_shot=*/false) != -ESRCH) {
if (walk_thread_list(mark_thread_to_die, &params, /*one_shot=*/false) != -ESRCH) {
killed = true;
}
DkThreadYieldExecution();
Expand All @@ -203,7 +219,7 @@ noreturn void process_exit(int error_code, int term_signal) {
thread_exit(error_code, term_signal);
}

(void)kill_other_threads();
(void)kill_other_threads(error_code, term_signal);

/* Now quit our thread. Since we are the last one, this will exit the whole LibOS. */
thread_exit(error_code, term_signal);
Expand Down
4 changes: 4 additions & 0 deletions LibOS/shim/test/ltp/ltp.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@
[DEFAULT]
timeout = 30

# no setsockopt(SOL_IP)
[accept02]
skip = yes

# utimensat
[access01]
skip = yes
Expand Down

0 comments on commit de6974a

Please sign in to comment.