Fix using a null item in target_handler::target_ui #36137
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
src/ranged.cpp
Outdated
| @@ -1324,8 +1323,9 @@ std::vector<tripoint> target_handler::target_ui( player &pc, target_mode mode, | |||
| aim_mode = aim_types.begin(); | |||
| } | |||
|
|
|||
| int num_instruction_lines = draw_targeting_window( w_target, relevant->tname(), | |||
| mode, ctxt, aim_types, tiny ); | |||
| // @TODO this assumes that relevant == null means firigin turrets, but that may not | |||
There was a problem hiding this comment.
Suggested change
| // @TODO this assumes that relevant == null means firigin turrets, but that may not | |
| // @TODO this assumes that relevant == null means firing turrets, but that may not |
src/ranged.cpp
Outdated
| @@ -1427,7 +1427,9 @@ std::vector<tripoint> target_handler::target_ui( player &pc, target_mode mode, | |||
| if( !compact ) { | |||
| line_number++; | |||
| } | |||
| if( mode == TARGET_MODE_FIRE || mode == TARGET_MODE_TURRET_MANUAL ) { | |||
| // Assumes that relevant == null means firing turrets (maybe even multiple at once), | |||
| // so printing their firing mode / ammo / ... of one of them is missleading. | |||
There was a problem hiding this comment.
Suggested change
| // so printing their firing mode / ammo / ... of one of them is missleading. | |
| // so printing their firing mode / ammo / ... of one of them is misleading. |
src/ranged.cpp
Outdated
| @@ -1470,7 +1472,9 @@ std::vector<tripoint> target_handler::target_ui( player &pc, target_mode mode, | |||
| c_red, '*' ); | |||
| } | |||
|
|
|||
| if( mode == TARGET_MODE_FIRE ) { | |||
| // Assumes that relevant == null means firing turrets (maybe even multiple at once), | |||
| // so printing their firing mode / ammo / ... of one of them is missleading. | |||
There was a problem hiding this comment.
Suggested change
| // so printing their firing mode / ammo / ... of one of them is missleading. | |
| // so printing their firing mode / ammo / ... of one of them is misleading. |
ZhilkinSerg
added
<Bugfix>
The pointer was dereferenced without any null-check anyway, so the function pretty much expects a valid pointer. This makes it clear to the caller.
Call `Character::has_item` instead of going the route via `get_item_position` (returns an invalid index) and calling `i_at` (which returns a null item when the input is an invalid index).
Instead of assuming it is `pc.weapon`. Note that `pc.weapon` may be a null item anyway (character needs to wield nothing when they want to fire a vehicle turret). This in turn means a null item was given to some functions around, and not all of them would handle this well. Some assumed the input was always a gun item.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY: NoneBasically if the function was not given a valid pointer, it would just use
g->u.weapon, which is often a null item (not wielding anything is required to fire a vehicle turret). In turn, some functions got called with that null item and some of them blindly assumes it will be a gun item and simply crashed when it wasn't.Fixes #35434.