From 3916acb1332f0c922d9261e790e86845ff931628 Mon Sep 17 00:00:00 2001 From: Goober5000 Date: Wed, 10 Jun 2026 23:56:59 -0400 Subject: [PATCH] put pause_parse/unpause_parse in RAII Replace pause_parse()/unpause_parse() with a PauseParseGuard class that bookmarks the parse state, points the parser at the supplied text and filename, and restores the bookmarked state on destruction or an explicit early unpause(). Lookahead sites construct the guard with the current parse position and rewind on unpause. Unlike pause_parse(), the guard also bookmarks Parse_text, so get_line_num() now walks the correct buffer during nested parses. Consequently, allocate_parse_text() now asserts that parsing is not paused, since clearing or reallocating the shared buffers mid-pause has always silently corrupted the paused parse. Also remove the unused text parameter from reset_parse(), which recreated the Parse_text/Mp mismatch with no bookmark to restore from. Co-Authored-By: Claude Fable 5 --- code/bmpman/bmpman.cpp | 13 +--- code/model/modelread.cpp | 18 ++--- code/parse/parselo.cpp | 57 +++++++++++---- code/parse/parselo.h | 35 ++++++++- code/parse/sexp/LuaAISEXP.cpp | 3 +- code/parse/sexp/LuaSEXP.cpp | 3 +- code/scripting/scripting.cpp | 5 +- code/ship/ship.cpp | 3 +- code/weapon/weapons.cpp | 4 +- test/src/parse/test_parselo.cpp | 73 +++++++++++++++++-- .../data/tables/test2.tbl | 2 + 11 files changed, 160 insertions(+), 56 deletions(-) create mode 100644 test/test_data/parselo/parse_pausing_exception_restores_state/data/tables/test2.tbl 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