coverity: possible use of invalid iterator (especially with jump nodes)

code/hud/hudbrackets.cpp

@@ -398,7 +398,6 @@ void HudGaugeBrackets::renderObjectBrackets(object *targetp, color *clr, int w_c
 	int x1,x2,y1,y2;
 	bool draw_box = true;
 	int bound_rc;
-	SCP_list<CJumpNode>::iterator jnp;
 
 
 	bool not_player_target = (Player_ai->target_objnum < 0 || targetp != &Objects[Player_ai->target_objnum]);
@@ -449,13 +448,11 @@ void HudGaugeBrackets::renderObjectBrackets(object *targetp, color *clr, int w_c
 			break;
 
 		case OBJ_JUMP_NODE:
-			for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) {
-				if(jnp->GetSCPObject() == targetp)
-					break;
-			}	
-				
-			modelnum = jnp->GetModelNumber();
+			{
+			auto jnp = jumpnode_get_by_objp(targetp);
+			modelnum = jnp ? jnp->GetModelNumber() : -1;
 			bound_rc = model_find_2d_bound_min( modelnum, &targetp->orient, &targetp->pos,&x1,&y1,&x2,&y2 );
+			}
 			break;
 
 		default:
@@ -637,7 +634,6 @@ void HudGaugeBrackets::renderBoundingBrackets(int x1, int y1, int x2, int y2, in
 		const char* tinfo_class = NULL;
 		char temp_name[NAME_LENGTH*2+3];
 		char temp_class[NAME_LENGTH];
-		SCP_list<CJumpNode>::iterator jnp;
 
 		switch(t_objp->type) {
 			case OBJ_SHIP:
@@ -676,11 +672,8 @@ void HudGaugeBrackets::renderBoundingBrackets(int x1, int y1, int x2, int y2, in
 				}
 				break;
 			case OBJ_JUMP_NODE:
-				for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) {
-					if(jnp->GetSCPObjectNumber() == target_objnum)
-						break;
-				}
-				tinfo_name = jnp->GetDisplayName();
+				auto jnp = jumpnode_get_by_objnum(target_objnum);
+				tinfo_name = jnp ? jnp->GetDisplayName() : "";
 				break;
 		}
 

code/hud/hudtarget.cpp

@@ -620,16 +620,11 @@ void hud_reticle_list_update(object *objp, float measure, int dot_flag)
 {
 	reticle_list	*rl, *new_rl;
 	int				i;
-	SCP_list<CJumpNode>::iterator jnp;
 
 	if (objp->type == OBJ_JUMP_NODE) {
-		for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) {
-			if( jnp->GetSCPObject() != objp )
-				continue;
-
-			if( jnp->IsHidden() )
-				return;
-		}
+		auto jnp = jumpnode_get_by_objp(objp);
+		if (!jnp || jnp->IsHidden())
+			return;
 	}
 
 	for ( rl = GET_FIRST(&Reticle_cur_list); rl != END_OF_LIST(&Reticle_cur_list); rl = GET_NEXT(rl) ) {
@@ -1168,7 +1163,6 @@ void hud_target_common(int team_mask, int next_flag)
 	object	*A, *start, *start2;
 	ship		*shipp;
 	int		is_ship, target_found = FALSE;
-	SCP_list<CJumpNode>::iterator jnp;
 
 	if (Player_ai->target_objnum == -1)
 		start = &obj_used_list;
@@ -1209,14 +1203,10 @@ void hud_target_common(int team_mask, int next_flag)
 		}
 
 		if (A->type == OBJ_JUMP_NODE) {
-			for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) {
-				if( jnp->GetSCPObject() == A )
-					break;
-			}
+			auto jnp = jumpnode_get_by_objp(A);
+			Assertion(jnp, "Failed to find jump node with object index %d; trace out and fix!\n", OBJ_INDEX(A));
 
-			Assertion(jnp != Jump_nodes.end(), "Failed to find jump node with object index %d; trace out and fix!\n", OBJ_INDEX(A));
-
-			if( jnp->IsHidden() )
+			if( !jnp || jnp->IsHidden() )
 				continue;
 		}
 
@@ -2419,7 +2409,6 @@ void hud_target_targets_target()
 int object_targetable_in_reticle(object *target_objp)
 {
 	int obj_type;
-	SCP_list<CJumpNode>::iterator jnp;
 
 	if (target_objp == Player_obj ) {
 		return 0;
@@ -2430,12 +2419,12 @@ int object_targetable_in_reticle(object *target_objp)
 	if ( (obj_type == OBJ_SHIP) || (obj_type == OBJ_DEBRIS) || (obj_type == OBJ_WEAPON) || (obj_type == OBJ_ASTEROID) )
 	{
 		return 1;
-	} else if ( obj_type == OBJ_JUMP_NODE )
+	}
+	else if ( obj_type == OBJ_JUMP_NODE )
 	{
-		for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) {
-			if(jnp->GetSCPObject() == target_objp)
-				break;
-		}
+		auto jnp = jumpnode_get_by_objp(target_objp);
+		if (!jnp)
+			return 0;
 
 		if (!jnp->IsHidden())
 			return 1;
@@ -2465,7 +2454,6 @@ void hud_target_in_reticle_new()
 	object	*A;
 	mc_info	mc;
 	float		dist;
-	SCP_list<CJumpNode>::iterator jnp;
 
 	hud_reticle_clear_list(&Reticle_cur_list);
 	Reticle_save_timestamp = timestamp(RESET_TARGET_IN_RETICLE);
@@ -2523,12 +2511,12 @@ void hud_target_in_reticle_new()
 			}
 			break;
 		case OBJ_JUMP_NODE:
-			for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) {
-				if(jnp->GetSCPObject() == A)
-					break;
-			}
-
+			{
+			auto jnp = jumpnode_get_by_objp(A);
+			if (!jnp)
+				continue;
 			mc.model_num = jnp->GetModelNumber();
+			}
 			break;
 		default:
 			Int3();	//	Illegal object type.

code/jumpnode/jumpnode.cpp

@@ -494,6 +494,24 @@ CJumpNode *jumpnode_get_by_objnum(int objnum)
 	return nullptr;
 }
 
+/**
+ * Get jump node object by the object pointer
+ *
+ * @param objp to search for
+ * @return Jump node object pointer
+ */
+CJumpNode *jumpnode_get_by_objp(const object *objp)
+{
+	Assert(objp != nullptr);
+
+	for (CJumpNode &jnp : Jump_nodes) {
+		if (jnp.GetSCPObject() == objp)
+			return &(jnp);
+	}
+
+	return nullptr;
+}
+
 /**
  * Given an object, returns which jump node it's inside (if any)
  *

code/jumpnode/jumpnode.h

@@ -93,6 +93,7 @@ extern SCP_list<CJumpNode> Jump_nodes;
 //-----Functions-----
 CJumpNode *jumpnode_get_by_name(const char *name);
 CJumpNode *jumpnode_get_by_objnum(int objnum);
+CJumpNode *jumpnode_get_by_objp(const object *objp);
 CJumpNode *jumpnode_get_which_in(const object *objp);
 
 void jumpnode_render_all();

code/radar/radarsetup.cpp

@@ -151,7 +151,6 @@ void radar_plot_object( object *objp )
 	vec3d pos, tempv;
 	float awacs_level, dist, max_radar_dist;
 	vec3d world_pos = objp->pos;
-	SCP_list<CJumpNode>::iterator jnp;
 
 	// don't process anything here.  Somehow, a jumpnode object caused this function
 	// to get entered on server side.
@@ -199,13 +198,10 @@ void radar_plot_object( object *objp )
 		
 		case OBJ_JUMP_NODE:
 		{
-			for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) {
-				if(jnp->GetSCPObject() == objp)
-					break;
-			}
+			auto jnp = jumpnode_get_by_objp(objp);
 			
-			// don't plot hidden jump nodes
-			if ( jnp->IsHidden() )
+			// don't plot missing or hidden jump nodes
+			if ( !jnp || jnp->IsHidden() )
 				return;
 
 			// filter jump nodes here if required
@@ -520,7 +516,6 @@ RadarVisibility radar_is_visible( object *objp )
 	vec3d pos, tempv;
 	float awacs_level, dist, max_radar_dist;
 	vec3d world_pos = objp->pos;
-	SCP_list<CJumpNode>::iterator jnp;
 
 	// get team-wide awacs level for the object if not ship
 	int ship_is_visible = 0;
@@ -558,13 +553,10 @@ RadarVisibility radar_is_visible( object *objp )
 		
 		case OBJ_JUMP_NODE:
 		{
-			for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) {
-				if(jnp->GetSCPObject() == objp)
-					break;
-			}
+			auto jnp = jumpnode_get_by_objp(objp);
 			
-			// don't plot hidden jump nodes
-			if ( jnp->IsHidden() )
+			// don't plot missing or hidden jump nodes
+			if ( !jnp || jnp->IsHidden() )
 				return NOT_VISIBLE;
 
 			// filter jump nodes here if required

code/scripting/api/libs/graphics.cpp

@@ -1087,7 +1087,6 @@ ADE_FUNC(drawTargetingBrackets, l_Graphics, "object Object, [boolean draw=true,
 	int bound_rc, pof;
 	int modelnum;
 	bool entered_frame = false;
-	SCP_list<CJumpNode>::iterator jnp;
 
 	if ( !(g3_in_frame( ) > 0 ) )
 	{
@@ -1132,15 +1131,16 @@ ADE_FUNC(drawTargetingBrackets, l_Graphics, "object Object, [boolean draw=true,
 			modelnum = Asteroid_info[Asteroids[targetp->instance].asteroid_type].subtypes[pof].model_number;
 			bound_rc = model_find_2d_bound_min( modelnum, &targetp->orient, &targetp->pos,&x1,&y1,&x2,&y2 );
 			break;
-		case OBJ_JUMP_NODE:
-			for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) {
-				if(jnp->GetSCPObject() == targetp)
-					break;
+		case OBJ_JUMP_NODE: {
+			auto jnp = jumpnode_get_by_objp(targetp);
+			if (!jnp) {
+				x1 = y1 = x2 = y2 = 0;
+				break;
 			}
-
 			modelnum = jnp->GetModelNumber();
 			bound_rc = model_find_2d_bound_min( modelnum, &targetp->orient, &targetp->pos,&x1,&y1,&x2,&y2 );
 			break;
+		}
 		default: //Someone passed an invalid pointer.
 			if ( entered_frame )
 				g3_end_frame( );

fred2/fredview.cpp

@@ -1429,7 +1429,6 @@ void CFREDView::OnContextMenu(CWnd* /*pWnd*/, CPoint point)
 	CMenu menu;
 	int	objnum;
 	CPoint local = point;
-	SCP_list<CJumpNode>::iterator jnp;
 
 	if (button_down) {
 		cancel_drag();
@@ -1455,11 +1454,10 @@ void CFREDView::OnContextMenu(CWnd* /*pWnd*/, CPoint point)
 					str.Format("Edit %s", Ships[Objects[objnum].instance].ship_name);
 
 				else if (Objects[objnum].type == OBJ_JUMP_NODE) {
+					auto jnp = jumpnode_get_by_objnum(objnum);
+					Assert(jnp != nullptr);
+
 					id = ID_EDITORS_JUMPNODE;
-					for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) {
-						if(jnp->GetSCPObject() == &Objects[objnum])
-							break;
-					}
 					str.Format("Edit %s", jnp->GetName());
 
 				} else if (Objects[objnum].type == OBJ_WAYPOINT) {

fred2/jumpnodedlg.cpp

@@ -127,17 +127,12 @@ void jumpnode_dlg::OnClose()
 void jumpnode_dlg::initialize_data(int full_update)
 {
 	int enable = TRUE;
-	SCP_list<CJumpNode>::iterator jnp;
 
 	if (!GetSafeHwnd())
 		return;
 
 	if (Objects[cur_object_index].type == OBJ_JUMP_NODE) {
-		for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) {
-			if(jnp->GetSCPObject() == &Objects[cur_object_index])
-				break;
-		}
-		
+		auto jnp = jumpnode_get_by_objnum(cur_object_index);
 		m_name = _T(jnp->GetName());
 		m_display = _T(jnp->GetDisplayName());
 
@@ -177,16 +172,12 @@ int jumpnode_dlg::update_data()
 	char old_name[255];
 	int i, z;
 	object *ptr;
-	SCP_list<CJumpNode>::iterator jnp;
 
 	if (!GetSafeHwnd())
 		return 0;
 
 	if (query_valid_object() && Objects[cur_object_index].type == OBJ_JUMP_NODE) {
-		for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) {
-			if(jnp->GetSCPObject() == &Objects[cur_object_index])
-				break;
-		}
+		auto jnp = jumpnode_get_by_objnum(cur_object_index);
 
 		for (i=0; i<MAX_WINGS; i++)
 		{

fred2/management.cpp

@@ -1264,7 +1264,8 @@ int common_object_delete(int obj)
 		Objects[obj].type = OBJ_NONE;
 		
 		// now call the destructor
-		Jump_nodes.erase(jnp);
+		if (jnp != Jump_nodes.end())
+			Jump_nodes.erase(jnp);
 
 		// now restore the jump node type so that the below unmark and obj_delete will work
 		Objects[obj].type = OBJ_JUMP_NODE;

qtfred/src/mission/Editor.cpp

@@ -1097,7 +1097,8 @@ int Editor::common_object_delete(int obj) {
 		Objects[obj].type = OBJ_NONE;
 
 		// now call the destructor
-		Jump_nodes.erase(jnp);
+		if (jnp != Jump_nodes.end())
+			Jump_nodes.erase(jnp);
 
 		// now restore the jump node type so that the below unmark and obj_delete will work
 		Objects[obj].type = OBJ_JUMP_NODE;

qtfred/src/mission/dialogs/WaypointEditorDialogModel.cpp

@@ -45,7 +45,6 @@ bool WaypointEditorDialogModel::apply() {
 	char old_name[255];
 	int i;
 	object* ptr;
-	SCP_list<CJumpNode>::iterator jnp;
 
 	if (query_valid_object(_editor->currentObject) && Objects[_editor->currentObject].type == OBJ_WAYPOINT) {
 		Assert(
@@ -141,11 +140,7 @@ bool WaypointEditorDialogModel::apply() {
 
 		_editor->missionChanged();
 	} else if (Objects[_editor->currentObject].type == OBJ_JUMP_NODE) {
-		for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) {
-			if (jnp->GetSCPObject() == &Objects[_editor->currentObject]) {
-				break;
-			}
-		}
+		auto jnp = jumpnode_get_by_objnum(_editor->currentObject);
 
 		for (i = 0; i < MAX_WINGS; i++) {
 			if (!stricmp(Wings[i].name, _currentName.c_str())) {
@@ -244,7 +239,6 @@ void WaypointEditorDialogModel::onSelectedObjectMarkingChanged(int, bool) {
 }
 void WaypointEditorDialogModel::initializeData() {
 	_enabled = true;
-	SCP_list<CJumpNode>::iterator jnp;
 
 	updateElementList();
 
@@ -256,13 +250,8 @@ void WaypointEditorDialogModel::initializeData() {
 	if (_editor->cur_waypoint_list != NULL) {
 		_currentName = _editor->cur_waypoint_list->get_name();
 	} else if (Objects[_editor->currentObject].type == OBJ_JUMP_NODE) {
-		for (jnp = Jump_nodes.begin(); jnp != Jump_nodes.end(); ++jnp) {
-			if (jnp->GetSCPObject() == &Objects[_editor->currentObject]) {
-				break;
-			}
-		}
-
-		_currentName = jnp->GetName();
+		auto jnp = jumpnode_get_by_objnum(_editor->currentObject);
+		_currentName = jnp ? jnp->GetName() : "";
 	} else {
 		_currentName = "";
 		_enabled = false;