Add undefined behaviour sanitizer to Travis CI (and fix issues it finds) #37384
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
When activating a sanitizer, explicitly request that we don't try to recover from a failure. This is so that when we run the tests in CI, ubsan issues will actually cause a failure. Otherwise they're just logged and no-one will notice them.
This member function of oracle_t was just a constant function. It didn't need to be a member function, so I made it a free function. In the tests it was being called on null oracle_t* values.
The crafting_success_roll could end up computing 0.0/0.0. This happened to work, but more by luck than design, so check for that case explicitly. (NaN values are unsafe given that we're compiling with -ffast-math).
In effect_type these bools would remain uninitialized for the derived class ma_buff_effect_type, leading to inconsistent behaviour of martial art effects. In inventory a newly constructed inventory could be considered binned, which could cause incorrect behaviour. In both cases, this involved loading invalid bool values, with is UB.
In some cases a call to get_overmaps_near was passing a tripoint with z value INT_MIN. This led to an overflow when computing the distance between points. Instead, just pass the x,y part of the tripoint.
Avoid division by zero in two cases: light level zero (used to check creature night sight on some code paths) and vehicle traction when all wheels are on the ground. This isn't actually UB, but in one case it led to UB because an infinity was cast to an int. Also, UBSan was complaining about it, possibly because we're doing things that are unsafe when using -ffast-math.
When a vehicle was being fast-forwarded over a period of months upon return to the reality bubble it was possible for the accumulated solar power to exceed billions of joules. This led to integer overflow. Perform intermediate calculation in 64-bit ints to avoid the problem.
The tests never initialized the state of the weather system, leaving some uninitialized values which could lead to winds in the billions of miles per hour. That in turn led to overflows and UB. Initialize the weather system appropriately.
By default, UBSan just prints a message but doesn't cause an error exit. This is not what we want for CI, so adding an extra option to make these errors fatal in order for CI issues to be obvious.
UBSan and ASan are compatible, so enable them both in the same test run.
jbytheway
changed the title
Using abs is in fact safe in this case, but causes a compiler warning.
ZhilkinSerg
added
[C++]
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
SUMMARY: Bugfixes "Various bugfixes found by undefined behaviour sanitizer"
Purpose of change
@hexagonrecursion found various issues in the game using UBSan. That reminded me that I had wanted to add it to Travis so we can run our tests with it enabled.
Describe the solution
The bugs which might have caused actual gameplay issues were:
effect_typeandinventory.get_overmaps_nearSome other bugs which would only occur in the tests were:
oracle_t*values inbehaviour_test.Finally, some things which UBSan was complaining about so I fixed them, but probably would have not actually caused any problems:
Describe alternatives you've considered
There's also the gcc ASan job, but I didn't enable it there because I think that one was closer to hitting the Travis job timeout. I couldn't find a good example Travis run to verify that, though.
Testing
Just running the unit tests
Additional context
The biggest risk here is that the tests and/or compile will go slower and not be possible under the Travis timeout. So hold off on merging until it becomes clear whether that's an issue. If need be I can disable some more of the slower tests for this specific run.
If we want the bugfixes in 0.E and it doesn't look like this PR will make it, then those can be cherry-picked as appropriate. Specifically, 31e9af9, 698fde7, and f572d0a are the more important fixes.