From 9efa897d0dc3ef1487d02e6a29e07690e9ca8321 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 21 Jan 2017 14:15:03 -0800 Subject: [PATCH] Early steps towards rationalizing SIGINT handling Previously we would try to walk all the blocks (from within the signal handler!) and mark them as skipped. Stop doing that, it's wildly unsafe. Also rationalize how the skip flag is set per block. Remove places that shouldn't set it (e.g. break and continue shouldn't set skip on the loop block). --- src/builtin.cpp | 10 ++++------ src/fish_tests.cpp | 1 + src/parse_execution.cpp | 10 +++------- src/parser.cpp | 37 +++++++++++-------------------------- src/parser.h | 2 +- 5 files changed, 20 insertions(+), 40 deletions(-) diff --git a/src/builtin.cpp b/src/builtin.cpp index 6ee68a005..9c6f33609 100644 --- a/src/builtin.cpp +++ b/src/builtin.cpp @@ -3033,15 +3033,14 @@ static int builtin_break_continue(parser_t &parser, io_streams_t &streams, wchar return STATUS_BUILTIN_ERROR; } - // Skip blocks interior to the loop. + // Skip blocks interior to the loop (but not the loop itself) size_t block_idx = loop_idx; while (block_idx--) { parser.block_at_index(block_idx)->skip = true; } - /* Skip the loop itself */ + // Mark the loop's status block_t *loop_block = parser.block_at_index(loop_idx); - loop_block->skip = true; loop_block->loop_status = is_break ? LOOP_BREAK : LOOP_CONTINUE; return STATUS_BUILTIN_OK; } @@ -3098,12 +3097,11 @@ static int builtin_return(parser_t &parser, io_streams_t &streams, wchar_t **arg return STATUS_BUILTIN_ERROR; } - // Skip everything up to (and then including) the function block. - for (size_t i = 0; i < function_block_idx; i++) { + // Skip everything up to and including the function block. + for (size_t i = 0; i <= function_block_idx; i++) { block_t *b = parser.block_at_index(i); b->skip = true; } - parser.block_at_index(function_block_idx)->skip = true; return status; } diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index e5d6d9b11..6fb542841 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -769,6 +769,7 @@ static void test_cancellation() { // Nasty infinite loop that doesn't actually execute anything. test_1_cancellation(L"echo (while true ; end) (while true ; end) (while true ; end)"); test_1_cancellation(L"while true ; end"); + test_1_cancellation(L"while true ; echo nothing > /dev/null; end"); test_1_cancellation(L"for i in (while true ; end) ; end"); // Restore signal handling. diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 3b10d2e08..72e0b72df 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -220,13 +220,12 @@ parse_execution_context_t::cancellation_reason(const block_t *block) const { if (parser && parser->cancellation_requested) { return execution_cancellation_skip; } - if (block && block->loop_status != LOOP_NORMAL) { - // Nasty hack - break and continue set the 'skip' flag as well as the loop status flag. - return execution_cancellation_loop_control; - } if (block && block->skip) { return execution_cancellation_skip; } + if (block && block->loop_status != LOOP_NORMAL) { + return execution_cancellation_loop_control; + } return execution_cancellation_none; } @@ -485,7 +484,6 @@ parse_execution_result_t parse_execution_context_t::run_for_statement( const wcstring &val = argument_sequence.at(i); env_set(for_var_name, val.c_str(), ENV_LOCAL); fb->loop_status = LOOP_NORMAL; - fb->skip = 0; this->run_job_list(block_contents, fb); @@ -494,7 +492,6 @@ parse_execution_result_t parse_execution_context_t::run_for_statement( if (fb->loop_status == LOOP_CONTINUE) { // Reset the loop state. fb->loop_status = LOOP_NORMAL; - fb->skip = false; continue; } else if (fb->loop_status == LOOP_BREAK) { break; @@ -656,7 +653,6 @@ parse_execution_result_t parse_execution_context_t::run_while_statement( if (wb->loop_status == LOOP_CONTINUE) { // Reset the loop state. wb->loop_status = LOOP_NORMAL; - wb->skip = false; continue; } else if (wb->loop_status == LOOP_BREAK) { break; diff --git a/src/parser.cpp b/src/parser.cpp index 0a7c77762..d50d9ecec 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -101,17 +101,13 @@ static wcstring user_presentable_path(const wcstring &path) { parser_t::parser_t() : cancellation_requested(false), is_within_fish_initialization(false) {} -/// A pointer to the principal parser (which is a static local). -static parser_t *s_principal_parser = NULL; + +static parser_t s_principal_parser; parser_t &parser_t::principal_parser(void) { ASSERT_IS_NOT_FORKED_CHILD(); ASSERT_IS_MAIN_THREAD(); - static parser_t parser; - if (!s_principal_parser) { - s_principal_parser = &parser; - } - return parser; + return s_principal_parser; } void parser_t::set_is_within_fish_initialization(bool flag) { @@ -120,14 +116,8 @@ void parser_t::set_is_within_fish_initialization(bool flag) { void parser_t::skip_all_blocks(void) { // Tell all blocks to skip. - if (s_principal_parser) { - s_principal_parser->cancellation_requested = true; - - // write(2, "Cancelling blocks\n", strlen("Cancelling blocks\n")); - for (size_t i = 0; i < s_principal_parser->block_count(); i++) { - s_principal_parser->block_at_index(i)->skip = true; - } - } + // This may be called from a signal handler! + s_principal_parser.cancellation_requested = true; } void parser_t::push_block(block_t *new_current) { @@ -139,22 +129,17 @@ void parser_t::push_block(block_t *new_current) { new_current->src_filename = intern(filename); } + // New blocks should be skipped if the outer block is skipped, except TOP and SUBST block, which + // open up new environments. const block_t *old_current = this->current_block(); - if (old_current && old_current->skip) { - new_current->skip = true; - } - - // New blocks should be skipped if the outer block is skipped, except TOP ans SUBST block, which - // open up new environments. Fake blocks should always be skipped. Rather complicated... :-( - new_current->skip = old_current ? old_current->skip : 0; + new_current->skip = old_current && old_current->skip; // Type TOP and SUBST are never skipped. if (type == TOP || type == SUBST) { - new_current->skip = 0; + new_current->skip = false; } - - new_current->job = 0; + new_current->job = nullptr; new_current->loop_status = LOOP_NORMAL; this->block_stack.push_back(new_current); @@ -782,7 +767,7 @@ void parser_t::get_backtrace(const wcstring &src, const parse_error_list_t &erro block_t::block_t(block_type_t t) : block_type(t), - skip(), + skip(false), tok_pos(), node_offset(NODE_OFFSET_INVALID), loop_status(LOOP_NORMAL), diff --git a/src/parser.h b/src/parser.h index 29d7f9452..380c70fcb 100644 --- a/src/parser.h +++ b/src/parser.h @@ -178,7 +178,7 @@ class parser_t { private: /// Indication that we should skip all blocks. - bool cancellation_requested; + volatile sig_atomic_t cancellation_requested; /// Indicates that we are within the process of initializing fish. bool is_within_fish_initialization; /// Stack of execution contexts. We own these pointers and must delete them.