Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't try to shoot with empty gun #77624

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

PatrikLundell
Copy link
Contributor

Summary

None

Purpose of change

Fix #77602, i.e. segmentation fault when trying to evaluate dispersion from an empty gun.

Describe the solution

Change npc_attack_gun::can_use to reject gun usage when a gun is empty, not only when it can't be wielded.
This causes the gun not to be attempted to be evaluated for firing performance down the line.

Describe alternatives you've considered

  • Leave it to someone qualified. Now those people will have to evaluate this PR instead.
  • Add a check in npc::wont_hit_friend to return "false" if there's no ammo. That also causes the symptoms to disappear, but thought it better to not evaluate the firing of an empty gun at all. Competent reviewers may disagree.

Testing

Loaded the save in the bug report and verified that not only didn't it blow up, but Liam did actually reload a gun (don't know if "varmint gun" is the same as the "Ruger" seen with the debugger), so the rejection doesn't seem to block gun usage to the degree that there is no attempt at reloading.

Additional context

The comment for the item::ammo_data() function is dangerously misleading. It claims it will return a nullptr if the gun isn't loaded, but that's a partial lie, as it happily returns data involving an empty magazine (although it probably rejects a direct loaded gun that's empty. Thus, anyone trusting enough to believe this may get a nasty surprise (segfault) when faced with an empty magazine and trying to use the ammo pointer that was promised to lead somewhere (it didn't matter here, as the ammo_data() results wasn't checked anyway).

Needs to be reviewed by competent reviewers. I THINK it's reasonable to bail out early, but it's possible this may lead to things that should be attempted being untried. Seeing the companion reload indicates at least one such possible failure seems to not happen in absolutely all cases.

@github-actions github-actions bot added NPC / Factions NPCs, AI, Speech, Factions, Ownership [C++] Changes (can be) made in C++. Previously named `Code` <Bugfix> This is a fix for a bug (or closes open issue) astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Nov 6, 2024
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions NPC / Factions NPCs, AI, Speech, Factions, Ownership
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segmentation fault
1 participant