fix some bugs and issues

Author: MjnMixael

code/missioneditor/sexp_tree_actions.cpp

@@ -73,7 +73,7 @@ void SexpTreeActions::replace_variable_data(int var_idx, int type)
 	clear_node_children(node_idx);
 
 	// Assemble name
-	sprintf(buf, "%s(%s)", Sexp_variables[var_idx].variable_name, Sexp_variables[var_idx].text);
+	snprintf(buf, sizeof(buf), "%s(%s)", Sexp_variables[var_idx].variable_name, Sexp_variables[var_idx].text);
 
 	_model.set_node(node_idx, type, buf);
 	void* h = _model.tree_nodes[node_idx].handle;
@@ -435,7 +435,7 @@ int SexpTreeActions::add_default_operator(int op_index, int argnum)
 			}
 
 			char node_text[2 * TOKEN_LENGTH + 2];
-			sprintf(node_text, "%s(%s)", item.text.c_str(), Sexp_variables[sexp_var_index].text);
+			snprintf(node_text, sizeof(node_text), "%s(%s)", item.text.c_str(), Sexp_variables[sexp_var_index].text);
 			add_variable_data(node_text, type);
 		}
 		// special case for sexps that take containers
@@ -489,18 +489,17 @@ int SexpTreeActions::add_default_operator(int op_index, int argnum)
 void SexpTreeActions::verify_and_fix_arguments(int node)
 {
 	int op_index, arg_num, type, tmp;
-	static int here_count = 0;
 	sexp_list_item* list;
 	sexp_list_item* ptr;
 	bool is_variable_arg = false;
 
-	if (here_count)
+	if (_verify_arguments_reentry_guard)
 		return;
 
-	here_count++;
+	_verify_arguments_reentry_guard++;
 	op_index = get_operator_index(_model.tree_nodes[node].text);
 	if (op_index < 0) {
-		here_count--;
+		_verify_arguments_reentry_guard--;
 		return;
 	}
 
@@ -509,6 +508,7 @@ void SexpTreeActions::verify_and_fix_arguments(int node)
 	arg_num = 0;
 	_model.item_index = _model.tree_nodes[node].child;
 	while (_model.item_index >= 0) {
+		is_variable_arg = false;
 		// get listing of valid argument values for node item_index
 		type = query_operator_argument_type(op_index, arg_num);
 		// special case for modify-variable
@@ -526,7 +526,7 @@ void SexpTreeActions::verify_and_fix_arguments(int node)
 			if (!list && (arg_num >= Operators[op_index].min)) {
 				_model.free_node(_model.item_index, 1);
 				_model.item_index = tmp;
-				here_count--;
+				_verify_arguments_reentry_guard--;
 				return;
 			}
 
@@ -558,6 +558,8 @@ void SexpTreeActions::verify_and_fix_arguments(int node)
 						}
 
 						if (types_match) {
+							list->destroy();
+							list = nullptr;
 							_model.item_index = _model.tree_nodes[_model.item_index].next;
 							arg_num++;
 							continue;
@@ -605,6 +607,11 @@ void SexpTreeActions::verify_and_fix_arguments(int node)
 
 			if (_model.tree_nodes[_model.item_index].type & SEXPT_OPERATOR)
 				verify_and_fix_arguments(_model.item_index);
+
+			if (list) {
+				list->destroy();
+				list = nullptr;
+			}
 		}
 
 		// fix the node if it is the argument for modify-variable
@@ -630,7 +637,7 @@ void SexpTreeActions::verify_and_fix_arguments(int node)
 	}
 
 	_model.item_index = tmp;
-	here_count--;
+	_verify_arguments_reentry_guard--;
 }
 
 // -----------------------------------------------------------------------
@@ -646,7 +653,7 @@ void SexpTreeActions::delete_sexp_tree_variable(const char* var_name)
 	char search_str[64];
 	char replace_text[TOKEN_LENGTH];
 
-	sprintf(search_str, "%s(", var_name);
+	snprintf(search_str, sizeof(search_str), "%s(", var_name);
 
 	int old_item_index = _model.item_index;
 
@@ -693,7 +700,7 @@ void SexpTreeActions::modify_sexp_tree_variable(const char* old_name, int sexp_v
 
 	int old_item_index = _model.item_index;
 
-	sprintf(search_str, "%s(", old_name);
+	snprintf(search_str, sizeof(search_str), "%s(", old_name);
 
 	for (int idx = 0; idx < static_cast<int>(_model.tree_nodes.size()); idx++) {
 		if (_model.tree_nodes[idx].type & SEXPT_VARIABLE) {

code/missioneditor/sexp_tree_actions.h

@@ -115,4 +115,5 @@ class SexpTreeActions {
 
 	SexpTreeModel& _model;  // shared tree node data and logic
 	ISexpTreeUI& _ui;       // UI callback interface for widget updates
+	int _verify_arguments_reentry_guard = 0;
 };

code/missioneditor/sexp_tree_model.cpp

@@ -36,15 +36,26 @@ constexpr int TREE_NODE_INCREMENT = 100;
 // If op_num is an op value (>= FIRST_OP), it is converted to an operator index first.
 void sexp_list_item::set_op(int op_num)
 {
-	int i;
+	int op_index = op_num;
 
 	if (op_num >= FIRST_OP) {  // do we have an op value instead of an op number (index)?
-		for (i = 0; i < static_cast<int>(Operators.size()); i++)
-			if (op_num == Operators[i].value)
-				op_num = i;  // convert op value to op number
+		op_index = -1;
+		for (int i = 0; i < static_cast<int>(Operators.size()); i++) {
+			if (op_num == Operators[i].value) {
+				op_index = i; // convert op value to op number
+				break;
+			}
+		}
 	}
 
-	op = op_num;
+	if (!SCP_vector_inbounds(Operators, op_index)) {
+		op = -1;
+		text = "<invalid operator>";
+		type = SEXPT_UNINIT;
+		return;
+	}
+
+	op = op_index;
 	text = Operators[op].text;
 	type = (SEXPT_OPERATOR | SEXPT_VALID);
 }
@@ -403,9 +414,9 @@ void get_combined_variable_name(char* combined_name, const char* sexp_var_name)
 	int sexp_var_index = get_index_sexp_variable_name(sexp_var_name);
 
 	if (sexp_var_index >= 0)
-		sprintf(combined_name, "%s(%s)", Sexp_variables[sexp_var_index].variable_name, Sexp_variables[sexp_var_index].text);
+		snprintf(combined_name, 2 * TOKEN_LENGTH + 2, "%s(%s)", Sexp_variables[sexp_var_index].variable_name, Sexp_variables[sexp_var_index].text);
 	else
-		sprintf(combined_name, "%s(undefined)", sexp_var_name);
+		snprintf(combined_name, 2 * TOKEN_LENGTH + 2, "%s(undefined)", sexp_var_name);
 }
 
 // Clear all tree nodes and reset counters. If 'op' is provided and non-empty,
@@ -848,6 +859,9 @@ int SexpTreeModel::query_restricted_opf_range(int opf)
 // Returns -1 if the node has no valid parent.
 int SexpTreeModel::get_sibling_place(int node) const
 {
+	if (!SCP_vector_inbounds(tree_nodes, node))
+		return -1;
+	
 	if (!SCP_vector_inbounds(tree_nodes, tree_nodes[node].parent))
 		return -1;
 
@@ -890,7 +904,7 @@ NodeImage SexpTreeModel::get_data_image(int node) const
 	int idx = (count % 100) / 5;
 
 	// There are 20 numbered data icons (DATA_00 through DATA_95)
-	if (idx > 20) {
+	if (idx >= 20) {
 		return NodeImage::DATA;
 	}
 
@@ -1032,7 +1046,7 @@ void get_variable_default_text_from_variable_text(char* text, char* default_text
 int SexpTreeModel::get_item_index_to_var_index() const
 {
 	// check valid item index and node is a variable
-	if ((item_index > 0) && (tree_nodes[item_index].type & SEXPT_VARIABLE)) {
+	if ((item_index >= 0) && SCP_vector_inbounds(tree_nodes, item_index) && (tree_nodes[item_index].type & SEXPT_VARIABLE)) {
 		return get_tree_name_to_sexp_variable_index(tree_nodes[item_index].text);
 	} else {
 		return -1;

code/missioneditor/sexp_tree_model.h

@@ -146,8 +146,6 @@ struct sexp_list_item {
 	void add_data(const char* str, int t = (SEXPT_STRING | SEXPT_VALID));
 	// Append another linked list to the end of this one
 	void add_list(sexp_list_item* list);
-	// Copy fields from src without transferring ownership of the next pointer
-	void shallow_copy(const sexp_list_item* src);
 	// Delete this item and all subsequent items in the linked list
 	void destroy();
 };
@@ -307,7 +305,7 @@ struct SexpContextMenuState {
 	// Campaign mode filtering
 	bool campaign_mode = false;
 
-	// Data lists for add/replace data submenus (caller must call destroy())
+	// Data lists for add/replace data submenus
 	// Entries with op >= 0 are operators to enable; op < 0 are data items for the submenu
 	sexp_list_item* add_data_list = nullptr;
 	int add_data_opf_type = 0;  // OPF_* type, needed for OPF_VARIABLE_NAME display
@@ -340,7 +338,66 @@ struct SexpContextMenuState {
 	bool show_container_data = false;
 	SCP_vector<ContainerEntry> replace_container_data;
 
-	// Free the dynamically allocated data lists (must be called when done with this state)
+	SexpContextMenuState() = default;
+	~SexpContextMenuState()
+	{
+		cleanup();
+	}
+	SexpContextMenuState(const SexpContextMenuState&) = delete;
+	SexpContextMenuState& operator=(const SexpContextMenuState&) = delete;
+	SexpContextMenuState(SexpContextMenuState&& other) noexcept
+	{
+		*this = std::move(other);
+	}
+	SexpContextMenuState& operator=(SexpContextMenuState&& other) noexcept
+	{
+		if (this != &other) {
+			cleanup();
+
+			is_labeled_root = other.is_labeled_root;
+			is_root_editable = other.is_root_editable;
+			can_edit_text = other.can_edit_text;
+			can_edit_comment = other.can_edit_comment;
+			can_edit_bg_color = other.can_edit_bg_color;
+			can_add_variable = other.can_add_variable;
+			can_modify_variable = other.can_modify_variable;
+			can_copy = other.can_copy;
+			can_cut = other.can_cut;
+			can_paste = other.can_paste;
+			can_paste_add = other.can_paste_add;
+			can_delete = other.can_delete;
+			can_replace_number = other.can_replace_number;
+			can_replace_string = other.can_replace_string;
+			can_add_number = other.can_add_number;
+			can_add_string = other.can_add_string;
+			add_type = other.add_type;
+			replace_type = other.replace_type;
+			insert_opf_type = other.insert_opf_type;
+			add_count = other.add_count;
+			replace_count = other.replace_count;
+			modify_variable = other.modify_variable;
+			campaign_mode = other.campaign_mode;
+			add_data_list = other.add_data_list;
+			add_data_opf_type = other.add_data_opf_type;
+			replace_data_list = other.replace_data_list;
+			add_enabled_op_indices = std::move(other.add_enabled_op_indices);
+			replace_enabled_op_indices = std::move(other.replace_enabled_op_indices);
+			op_add_enabled = std::move(other.op_add_enabled);
+			op_replace_enabled = std::move(other.op_replace_enabled);
+			op_insert_enabled = std::move(other.op_insert_enabled);
+			replace_variables = std::move(other.replace_variables);
+			show_container_names = other.show_container_names;
+			replace_container_names = std::move(other.replace_container_names);
+			show_container_data = other.show_container_data;
+			replace_container_data = std::move(other.replace_container_data);
+
+			other.add_data_list = nullptr;
+			other.replace_data_list = nullptr;
+		}
+		return *this;
+	}
+
+	// Free the dynamically allocated data lists (safe to call multiple times)
 	void cleanup() {
 		if (add_data_list) { add_data_list->destroy(); add_data_list = nullptr; }
 		if (replace_data_list) { replace_data_list->destroy(); replace_data_list = nullptr; }
@@ -506,7 +563,7 @@ class SexpTreeModel {
 
 	// Analyze the current selection and compute which context menu actions are available.
 	// The returned state includes operator enablement, variable/container menus, and
-	// clipboard paste validation. Caller must call cleanup() on the result.
+	// clipboard paste validation. Returned state owns temporary lists and cleans them up automatically.
 	SexpContextMenuState compute_context_menu_state();
 	// Returns true if the given operator value should be hidden from menus (deprecated/hidden ops)
 	static bool is_operator_hidden(int op_value);

code/missioneditor/sexp_tree_opf.cpp

@@ -199,7 +199,10 @@ sexp_list_item *SexpTreeOPF::get_listing_opf_prop()
 	ptr = GET_FIRST(&obj_used_list);
 	while (ptr != END_OF_LIST(&obj_used_list)) {
 		if (ptr->type == OBJ_PROP) {
-			head.add_data(prop_id_lookup(ptr->instance)->prop_name);
+			auto prop = prop_id_lookup(ptr->instance);
+			if (prop != nullptr) {
+				head.add_data(prop->prop_name);
+			}
 		}
 
 		ptr = GET_NEXT(ptr);
@@ -477,9 +480,15 @@ sexp_list_item *SexpTreeOPF::get_listing_opf_docker_point(int parent_node, int a
 
 	if (sh >= 0)
 	{
-		polymodel *pm = model_get(Ship_info[Ships[sh].ship_info_index].model_num);
-		for (int i = 0; i < pm->n_docks; i++)
-			head.add_data(pm->docking_bays[i].name);
+		auto model_num = Ship_info[Ships[sh].ship_info_index].model_num;
+		if (model_num >= 0) {
+			polymodel* pm = model_get(model_num);
+			if (pm != nullptr) {
+				for (int i = 0; i < pm->n_docks; i++) {
+					head.add_data(pm->docking_bays[i].name);
+				}
+			}
+		}
 	}
 
 	return head.next;
@@ -517,9 +526,15 @@ sexp_list_item *SexpTreeOPF::get_listing_opf_dockee_point(int parent_node)
 
 	if (sh >= 0)
 	{
-		polymodel *pm = model_get(Ship_info[Ships[sh].ship_info_index].model_num);
-		for (int i = 0; i < pm->n_docks; i++)
-			head.add_data(pm->docking_bays[i].name);
+		auto model_num = Ship_info[Ships[sh].ship_info_index].model_num;
+		if (model_num >= 0) {
+			polymodel* pm = model_get(model_num);
+			if (pm != nullptr) {
+				for (int i = 0; i < pm->n_docks; i++) {
+					head.add_data(pm->docking_bays[i].name);
+				}
+			}
+		}
 	}
 
 	return head.next;
@@ -1429,11 +1444,17 @@ sexp_list_item *SexpTreeOPF::get_listing_opf_animation_name(int parent_node)
 	if (child < 0)
 		return nullptr;
 	sh = ship_name_lookup(_model.tree_nodes[child].text, 1);
+	if (sh < 0) {
+		return nullptr;
+	}
 
 	switch(op) {
 		case OP_TRIGGER_ANIMATION_NEW:
 		case OP_STOP_LOOPING_ANIMATION: {
 			child = _model.tree_nodes[child].next;
+			if (child < 0) {
+				return head.next;
+			}
 			auto triggerType = animation::anim_match_type(_model.tree_nodes[child].text);
 
 			for (const auto& anim_ref : Ship_info[Ships[sh].ship_info_index].animations.getRegisteredTriggers()) {