diff --git a/code/bmpman/bmpman.cpp b/code/bmpman/bmpman.cpp index 0c0743d03e3..fd540b3478c 100644 --- a/code/bmpman/bmpman.cpp +++ b/code/bmpman/bmpman.cpp @@ -1349,14 +1349,13 @@ bool bm_load_and_parse_eff(const char *filename, int dir_type, int *nframes, int memset(file_text, 0, sizeof(file_text)); memset(file_text_raw, 0, sizeof(file_text_raw)); - // pause anything that may happen to be parsing right now - pause_parse(); - try { - // now start parsing the EFF + // read the EFF into our own buffers so we don't disturb anything that may happen to be parsing right now read_file_text(filename, dir_type, file_text, file_text_raw); - reset_parse(file_text); + + // pause any current parsing and start parsing the EFF; parsing unpauses when the guard goes out of scope + PauseParseGuard guard(file_text, filename); required_string("$Type:"); stuff_string(ext, F_NAME, sizeof(ext)); @@ -1373,13 +1372,9 @@ bool bm_load_and_parse_eff(const char *filename, int dir_type, int *nframes, int catch (const parse::ParseException& e) { mprintf(("BMPMAN: Unable to parse '%s'! Error message = %s.\n", filename, e.what())); - unpause_parse(); return false; } - // done with EFF so unpause parsing so whatever can continue - unpause_parse(); - if (!stricmp(NOX("dds"), ext)) { c_type = BM_TYPE_DDS; } else if (!stricmp(NOX("tga"), ext)) { diff --git a/code/model/modelread.cpp b/code/model/modelread.cpp index c5f908f7c71..fec505116d3 100644 --- a/code/model/modelread.cpp +++ b/code/model/modelread.cpp @@ -433,9 +433,9 @@ bool get_user_vec3d_value(char *buf, vec3d *value, bool require_brackets, const char closing_bracket = '\0'; bool success = false; - pause_parse(); - Mp = buf; - snprintf(Current_filename, sizeof(Current_filename), "submodel %s on %s", submodel_name, filename); + char pseudo_filename[MAX_PATH_LEN]; + snprintf(pseudo_filename, sizeof(pseudo_filename), "submodel %s on %s", submodel_name, filename); + PauseParseGuard guard(buf, pseudo_filename); // Check if there's a missing line break before the next "$". char end_separator = '\0'; @@ -450,8 +450,8 @@ bool get_user_vec3d_value(char *buf, vec3d *value, bool require_brackets, const } // Note that we can't simply return from within this block - // because we always need to call unpause_parse before we - // leave the function. A one-iteration loop with break + // because we always need to revert the end_separator replacement + // before we leave the function. A one-iteration loop with break // statements allows the code to jump to the end. Alternatively, // goto could have been used. do { @@ -495,7 +495,6 @@ bool get_user_vec3d_value(char *buf, vec3d *value, bool require_brackets, const *end_pos = end_separator; } - unpause_parse(); return success; } @@ -3275,9 +3274,8 @@ int model_load(const char* filename, ship_info* sip, ErrorType error_type, bool if (!VALID_FNAME(filename)) { return -1; } - pause_parse(); - Mp = Parse_text; - strcpy_s(Current_filename, filename); + // not parsing anything from Parse_text, but this makes parse errors report the model filename + PauseParseGuard guard(Parse_text, filename); TRACE_SCOPE(tracing::LoadModelFile); @@ -3322,7 +3320,6 @@ int model_load(const char* filename, ship_info* sip, ErrorType error_type, bool } Polygon_models[num] = NULL; - unpause_parse(); return -1; } @@ -3521,7 +3518,6 @@ int model_load(const char* filename, ship_info* sip, ErrorType error_type, bool model_set_subsys_path_nums(pm, n_subsystems, subsystems); model_set_bay_path_nums(pm); - unpause_parse(); if (sip != nullptr) sip->model_num = pm->id; return pm->id; diff --git a/code/parse/parselo.cpp b/code/parse/parselo.cpp index c4076577b9f..0df2d58dcb6 100644 --- a/code/parse/parselo.cpp +++ b/code/parse/parselo.cpp @@ -2376,6 +2376,11 @@ void allocate_parse_text(size_t size) { Assert( size > 0 ); + // while parsing is paused, the shared buffers are referenced by the bookmark + // stack (and Parse_text may not even point at them), so clearing or + // reallocating them would corrupt the paused parse + Assertion( Bookmarks.empty(), "allocate_parse_text() must not be called while parsing is paused!" ); + // Make sure that there is space for the terminating null character size += 1; @@ -3638,29 +3643,54 @@ void find_and_stuff_or_add(const char *id, int *addr, int f_type, char *strlist[ } } -// pause current parsing so that some else can be parsed without interfering -// with the currently parsing file -void pause_parse() +PauseParseGuard::PauseParseGuard(char *text, const char *filename) { - Bookmark Mark; + // text may be null when nothing is being parsed, but a filename is always needed for error reporting + Assertion(filename != nullptr, "PauseParseGuard filename must not be null!"); + // pause current parsing so that the supplied text can be parsed without interfering with the currently parsing file + Bookmark Mark; Mark.filename = Current_filename; + Mark.Parse_text = Parse_text; Mark.Mp = Mp; Mark.Warning_count = Warning_count; Mark.Error_count = Error_count; - Bookmarks.push_back(std::move(Mark)); + m_depth = Bookmarks.size(); + + // point the parser at the new text. Parse_text must track the start of the buffer containing Mp so that get_line_num() walks the right buffer. + // If text is the parser's current position (the lookahead idiom), the position is still within the current buffer, so the buffer start is + // unchanged; otherwise the supplied text becomes the new buffer. + if (text != Mp) + Parse_text = text; + Mp = text; + Warning_count = 0; + Error_count = 0; + + // install the new filename, truncating if necessary; skip the copy when the caller passes Current_filename itself to bookmark the current position + if (filename != Current_filename) + snprintf(Current_filename, sizeof(Current_filename), "%s", filename); } -// unpause parsing to continue with previously parsing file -void unpause_parse() +PauseParseGuard::~PauseParseGuard() { - Assert( !Bookmarks.empty() ); + unpause(); +} + +// unpause parsing to continue with the previously parsing file +void PauseParseGuard::unpause() +{ + if (!m_active) + return; + m_active = false; + + Assertion(Bookmarks.size() == m_depth, "Bookmark stack mismatch: expected depth " SIZE_T_ARG " but found " SIZE_T_ARG ". Something between pausing and unpausing left the stack unbalanced.", m_depth, Bookmarks.size()); if (Bookmarks.empty()) return; - Bookmark Mark = Bookmarks.back(); + const Bookmark &Mark = Bookmarks.back(); + Parse_text = Mark.Parse_text; Mp = Mark.Mp; Warning_count = Mark.Warning_count; Error_count = Mark.Error_count; @@ -3670,13 +3700,10 @@ void unpause_parse() Bookmarks.pop_back(); } -void reset_parse(char *text) +// restart parsing at the beginning of the parse text; to parse some other text, use PauseParseGuard +void reset_parse() { - if (text != NULL) { - Mp = text; - } else { - Mp = Parse_text; - } + Mp = Parse_text; Warning_count = 0; Error_count = 0; diff --git a/code/parse/parselo.h b/code/parse/parselo.h index 8640962302d..55324dbeec4 100644 --- a/code/parse/parselo.h +++ b/code/parse/parselo.h @@ -348,10 +348,38 @@ extern void parse_int_list(int *ilist, size_t size); extern void parse_string_map(SCP_map& mapOut, const char* end_marker, const char* entry_prefix); // general -extern void reset_parse(char *text = NULL); +extern void reset_parse(); extern void display_parse_diagnostics(); -extern void pause_parse(); -extern void unpause_parse(); + +// RAII guard that pauses the current parse so that some other text can be parsed +// without interfering with the currently parsing file. The constructor bookmarks +// the current parse state (buffer, position, filename, and warning/error counts) +// on a stack, then points the parser at the supplied text and filename with zeroed +// counts. The destructor (or an explicit unpause() call) restores the bookmarked +// state, even if the parse path exits via an exception. Guards nest in LIFO order. +// +// To bookmark the current position for lookahead, pass the parser's own state: +// PauseParseGuard guard(Mp, Current_filename); +// Unpausing rewinds the parser to the bookmarked position. +class PauseParseGuard +{ +public: + PauseParseGuard(char *text, const char *filename); + ~PauseParseGuard(); + + // restore the bookmarked parse state now, rather than at end of scope; + // subsequent calls (including the destructor's) are no-ops + void unpause(); + + PauseParseGuard(const PauseParseGuard &) = delete; + PauseParseGuard &operator=(const PauseParseGuard &) = delete; + PauseParseGuard(PauseParseGuard &&) = delete; + PauseParseGuard &operator=(PauseParseGuard &&) = delete; + +private: + size_t m_depth = 0; + bool m_active = true; +}; // stop parsing, basically just frees up the memory from Parse_text and Parse_text_raw extern void stop_parse(); @@ -499,6 +527,7 @@ namespace parse { public: SCP_string filename; + char* Parse_text; char* Mp; int Warning_count; int Error_count; diff --git a/code/parse/sexp/LuaAISEXP.cpp b/code/parse/sexp/LuaAISEXP.cpp index 55181774fe6..271857f47ae 100644 --- a/code/parse/sexp/LuaAISEXP.cpp +++ b/code/parse/sexp/LuaAISEXP.cpp @@ -51,7 +51,7 @@ bool LuaAISEXP::parseCheckEndOfDescription() { // Since we're stuffing strings, this is the best way to "unstuff" a string // if we determine we're finished with the description - and we also want // to preserve whitespace (which the parser eats) while building the description - pause_parse(); + PauseParseGuard guard(Mp, Current_filename); // bookmark the current position for lookahead; rewinds when the guard goes out of scope // look for any token that can follow $Description auto possible_tokens = @@ -76,7 +76,6 @@ bool LuaAISEXP::parseCheckEndOfDescription() { } } - unpause_parse(); return found; } diff --git a/code/parse/sexp/LuaSEXP.cpp b/code/parse/sexp/LuaSEXP.cpp index 0fbf208cc84..47e50247c9f 100644 --- a/code/parse/sexp/LuaSEXP.cpp +++ b/code/parse/sexp/LuaSEXP.cpp @@ -537,7 +537,7 @@ bool LuaSEXP::parseCheckEndOfDescription() { // Since we're stuffing strings, this is the best way to "unstuff" a string // if we determine we're finished with the description - and we also want // to preserve whitespace (which the parser eats) while building the description - pause_parse(); + PauseParseGuard guard(Mp, Current_filename); // bookmark the current position for lookahead; rewinds when the guard goes out of scope // look for any token that can follow $Description auto possible_tokens = @@ -558,7 +558,6 @@ bool LuaSEXP::parseCheckEndOfDescription() { } } - unpause_parse(); return found; } void LuaSEXP::parseTable() { diff --git a/code/scripting/scripting.cpp b/code/scripting/scripting.cpp index 93ba85a6b24..06b45aa0ad2 100644 --- a/code/scripting/scripting.cpp +++ b/code/scripting/scripting.cpp @@ -935,9 +935,8 @@ bool script_state::ParseCondition(const char *filename) sat.global_conditions = parsed_conditions; for (const SCP_string& local_condition : conditions) { bool found = false; - pause_parse(); SCP_vm_unique_ptr parse{ vm_strdup(local_condition.c_str()) }; - reset_parse(parse.get()); // coverity[escape:FALSE] - this is okay because the pointer escape only lasts until unpause_parse() restores the old state + PauseParseGuard guard(parse.get(), buf.c_str()); // coverity[escape:FALSE] - this is okay because the pointer escape only lasts until the guard restores the old state for (const auto& potential_condition : currHook->_conditions) { SCP_string bufCond; sprintf(bufCond, "$%s:", potential_condition.first.c_str()); @@ -949,7 +948,7 @@ bool script_state::ParseCondition(const char *filename) break; } } - unpause_parse(); + guard.unpause(); if (!found) { error_display(0, "Condition '%s' is not valid for hook '%s'. The hook will not evaluate!", local_condition.c_str(), currHook->getHookName().c_str()); diff --git a/code/ship/ship.cpp b/code/ship/ship.cpp index 31485fc7c2d..ce1618fbd9a 100644 --- a/code/ship/ship.cpp +++ b/code/ship/ship.cpp @@ -3850,7 +3850,7 @@ static void parse_ship_values(ship_info* sip, const bool is_template, const bool // figure out whether this is a supercap by doing some parse gymnastics // (flags are parsed several lines later, so we need to skip ahead, peek at the flags, and jump back) - pause_parse(); + PauseParseGuard guard(Mp, Current_filename); // bookmark the current position for lookahead; rewinds on unpause if (skip_to_string("$Flags:", "$Name:") == 1) { // cache the flag definition so we don't have to keep looking it up static auto supercap_flag_def = std::find_if(std::begin(Ship_flags), std::end(Ship_flags), [](const flag_def_list_new &item) { @@ -3869,7 +3869,6 @@ static void parse_ship_values(ship_info* sip, const bool is_template, const bool } } } - unpause_parse(); } // get ship parameters for warpin and warpout diff --git a/code/weapon/weapons.cpp b/code/weapon/weapons.cpp index 1534f092a4d..8de279481f3 100644 --- a/code/weapon/weapons.cpp +++ b/code/weapon/weapons.cpp @@ -410,7 +410,7 @@ void parse_wi_flags(weapon_info *weaponp) // have to do this slightly out of order in order to handle spawn properly // skip OVER flags, check for +override and reset num_spawn_weapons_defined // otherwise if done afterward, new and old spawn weapons can get mixed up - pause_parse(); + PauseParseGuard guard(Mp, Current_filename); // bookmark the current position for lookahead; rewinds on unpause SCP_vector flags; stuff_string_list(flags); if (optional_string("+override")) { @@ -445,7 +445,7 @@ void parse_wi_flags(weapon_info *weaponp) weaponp->wi_flags = cleared_wi_flags; weaponp->num_spawn_weapons_defined = 0; } - unpause_parse(); + guard.unpause(); // rewind so the flag list can be parsed for real // To make sure +override doesn't overwrite previously parsed values we parse the flags into a separate flagset SCP_vector unparsed; diff --git a/test/src/parse/test_parselo.cpp b/test/src/parse/test_parselo.cpp index 40daa020bb5..b6a6a9384ad 100644 --- a/test/src/parse/test_parselo.cpp +++ b/test/src/parse/test_parselo.cpp @@ -29,24 +29,83 @@ TEST_F(ParseloTest, parse_pausing) { // Consume a token required_string("#Start"); - // Now pause parsing and check if the right text is loaded + // Read the nested file into our own buffers so the outer parse is not disturbed char file_text[1024]; char file_text_raw[1024]; memset(file_text, 0, sizeof(file_text)); memset(file_text_raw, 0, sizeof(file_text_raw)); - pause_parse(); + read_file_text("test2.tbl", CF_TYPE_TABLES, file_text, file_text_raw); // NOLINT(readability-suspicious-call-argument) - read_file_text("test2.tbl", CF_TYPE_TABLES, file_text, file_text_raw); - reset_parse(file_text); + { + // Now pause parsing and check if the right text is loaded + PauseParseGuard guard(file_text, "test2.tbl"); - required_string("#Begin"); + // Line numbers should be relative to the nested file, which requires + // Parse_text to track the buffer containing Mp + required_string("#Begin"); + EXPECT_EQ(get_line_num(), 1); + required_string("#End"); + EXPECT_EQ(get_line_num(), 2); + } + + // We should be back in the original file + required_string("$Token:"); + EXPECT_EQ(get_line_num(), 3); + required_string("+OtherToken:"); required_string("#End"); +} - unpause_parse(); +TEST_F(ParseloTest, parse_pausing_exception_restores_state) { + read_file_text("test.tbl", CF_TYPE_TABLES); + reset_parse(); - // We should be back in the original file + required_string("#Start"); + + char file_text[1024]; + char file_text_raw[1024]; + + memset(file_text, 0, sizeof(file_text)); + memset(file_text_raw, 0, sizeof(file_text_raw)); + + read_file_text("test2.tbl", CF_TYPE_TABLES, file_text, file_text_raw); // NOLINT(readability-suspicious-call-argument) + + bool caught = false; + try { + PauseParseGuard guard(file_text, "test2.tbl"); + required_string("#Begin"); + throw parse::ParseException("simulated parse failure"); + } catch (const parse::ParseException&) { + caught = true; + } + ASSERT_TRUE(caught); + + // The guard's destructor should have restored the outer file during stack unwinding + required_string("$Token:"); + required_string("+OtherToken:"); + required_string("#End"); +} + +TEST_F(ParseloTest, parse_pausing_early_unpause) { + read_file_text("test.tbl", CF_TYPE_TABLES); + reset_parse(); + + required_string("#Start"); + + { + // Bookmark the current position for lookahead + PauseParseGuard guard(Mp, Current_filename); + required_string("$Token:"); + + // Rewind to the bookmarked position + guard.unpause(); + + // A second unpause is a silent no-op + guard.unpause(); + } // ...and so is the destructor + + // The lookahead should have been rewound, so the token is still pending required_string("$Token:"); required_string("+OtherToken:"); required_string("#End"); diff --git a/test/test_data/parselo/parse_pausing_exception_restores_state/data/tables/test2.tbl b/test/test_data/parselo/parse_pausing_exception_restores_state/data/tables/test2.tbl new file mode 100644 index 00000000000..e5180e330ec --- /dev/null +++ b/test/test_data/parselo/parse_pausing_exception_restores_state/data/tables/test2.tbl @@ -0,0 +1,2 @@ +#Begin +#End \ No newline at end of file