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

Avoid divide by zero in vehicle::print_fuel_indicator #64409

Merged
merged 1 commit into from
Mar 21, 2023

Conversation

pjf
Copy link
Contributor

@pjf pjf commented Mar 20, 2023

Summary

None

Purpose of change

Stop crash to desktop on examining some vehicles.

Fixes #64407.

Describe the solution

Checked if the fuel cap for a given fuel was zero, and avoided showing/calculating time-to-full/empty calculations if it is.

Describe alternatives you've considered

None.

Testing

Loaded save in #64407 without patch applied. Game crashes on vehicle examination.

Loaded save in #64407 with patch applied. Vehicle can be examined fine.

Additional context

None.

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` Vehicles Vehicles, parts, mechanics & interactions <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 Mar 20, 2023
@irwiss
Copy link
Contributor

irwiss commented Mar 20, 2023

I can't reproduce the crash using the save from linked PR on commit b738ae2
I think the actual repro case happened on a weirdly generated wreck and failed to survive a save/load cycle
Likely it's caused by something I changed in #64097 though

I think it might be worth it to do the cap == 0 case handling earlier; since with verbose == true and debug_mode == false the division by 0 will happen on line 437

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Mar 20, 2023
@pjf
Copy link
Contributor Author

pjf commented Mar 20, 2023

@irwiss : Well heck, I've now tried it with the save as well and can't reproduce it either. A weird wreck that didn't survive a save/load cycle makes perfect sense, especially since weird wrecks have been the bane of my existence.

Adding more comments in #64407.

@irwiss
Copy link
Contributor

irwiss commented Mar 20, 2023

I mean regardless of reproduction it's probably a good idea to block division by zero, but your solution misses the case you described in the issue:
When examining vehicle the function is called with verbose=true, and unless you're in debug mode you'll divide at line 437 before you get to the check you added

Cap can be zero on wrecks, so it's important we check for it.

Fixes CleverRaven#64407
@pjf pjf force-pushed the pull/fix_veh_div_by_zero branch from 08004fc to 7945343 Compare March 21, 2023 01:05
@pjf
Copy link
Contributor Author

pjf commented Mar 21, 2023

When examining vehicle the function is called with verbose=true, and unless you're in debug mode you'll divide at line 437 before you get to the check you added

Oh gosh, you're absolutely right. PR updated.

@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Mar 21, 2023
@kevingranade kevingranade merged commit 66a4e1f into CleverRaven:master Mar 21, 2023
@pjf pjf deleted the pull/fix_veh_div_by_zero branch March 21, 2023 23:41
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 <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 Vehicles Vehicles, parts, mechanics & interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SIGFPE crash in vehicle::print_fuel_indicator
3 participants