Resolve 2 UBSAN warnings about uninitialized vars; establish allocation/type check along with it #574
+519
−515
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
Lgt2x
reviewed
Sep 9, 2024
|
Ok to merge when trivial conflict with logging module is solved |
UBSAN is reporting that some uninitialized variables. To that end, I would like to add member default initializers to e.g. ``struct object``. Doing that makes classes nontrivial. Allocations throughout the code occur with e.g. ``mem_malloc(sizeof(T))``, which is, tersely speaking, incompatible with triviality. Therefore, mem_malloc call sites will be rewritten to invoke ``mem_rmalloc<T>`` instead, to enforce a compile-time check that ``T`` is indeed trivial. ``mem_rmalloc<T>`` is modeled to look like ``make_unique<T>``, i.e. can be called with no arguments to allocate space for a single thing, or called with a size_t argument that specifies the number of elements (automatic byte sizing is applied internally).
git grep -l mem_malloc | xargs perl -i -lpe 's{\((\w+) \*\)mem_malloc\(sizeof\(\1\)\)}{mem_rmalloc<$1>()}'
Automated replacement with
```
git grep -l mem_malloc |
xargs perl -i -lpe 's{\((\w+) \*\)mem_malloc\(sizeof\(\1\) \* (\S+)\)}{mem_rmalloc<$1>($2)}'
```
```
git grep -l mem_malloc | xargs perl -i -lpe 's{\((\w+) \*\*\)mem_malloc\(sizeof\(\1 \*\) \* (\S+)\)}{mem_rmalloc<$1>($2)}'
```
```
git grep -l mem_malloc | xargs perl -i -lpe 's{\((\w+) \*\)mem_malloc\((\S+) \* sizeof\(\1\)\)}{mem_rmalloc<$1 *>($2)}'
```
```
git grep -l mem_malloc | xargs perl -i -lpe 's{\((\w+) \*\*\)mem_malloc\((\S+) \* sizeof\(\1 \*\)\)}{mem_rmalloc<$1 *>($2)}'
```
```
git grep -l mem_malloc | xargs perl -i -lpe 's{\((uint8_t) \*\)mem_malloc\((\S+)\)}{mem_rmalloc<$1>($2)}'
```
```
git grep -l mem_malloc | xargs perl -i -lpe 's{\((char) \*\)mem_malloc\((\S+)\)}{mem_rmalloc<$1>($2)}'
```
(Manual modification.) Change mem_malloc callsites with non-fundamental types to mem_rmalloc.
lnxgameController::flush accesses this->m_MouseActive before that member being initialized with something sensible, making ASAN report: linux/lnxcontroller.cpp:259:8: runtime error: load of value 190, which is not a valid value for type 'bool' linux/lnxcontroller.cpp:259:33: runtime error: load of value 190, which is not a valid value for type 'bool' Thanks to the introduction of mem_rmalloc, it has been established that struct lnxgameController is not malloc'd anywhere, so any instantiation runs its constructor properly.
UBSAN warns: $GIT/Descent3/object.cpp:2452:91: runtime error: load of value 2, which is not a valid value for type 'bool'
jengelh
force-pushed
the
uninit_vars
branch
from
bc2bc2f to
08004b9
Compare
20 tasks
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.
Pull Request Type
Description
UBSAN evaluates use of undefined variable contents at runtime; it's more effective against unintialized heap allocations, whereas compile-time is more effective to mark code paths that are perhaps not exercised during gameplay at all.
mem_malloc is rewritten to catch potentially allocating nontrivial class types, the latter of which is a result of adding initializers. This supersedes 7777720 which only fixed a few struct members.
Related Issues
#568
Checklist