Fix NULL Map dereference in MapIterate#34
Conversation
…er to get this compiling in an hour or so at most. These changes get the x64 debug build working for VS 2022 community edition. 0. Completely ignore `build.bat`. 1. Change the incursion lib project to not run modaccent or flex. modaccent x64 will crash when running, likely it might still work for x86. See Language Files folder in the project, `grammar.acc` and `tokens.lex`. Right click on each, Properties and exclude from build. 2. Download libtcod-1.7.0-x86_64-msvc binaries. 3. Extract into build\libtcod-1.7.0-x86_64-msvc. 4. Compile `exe_libtcod` project. 5. Debug the `exe_libtcod` project and observe it runs and you see the libtcod game. Exit. 6. Copy `build\libtcod-1.7.0-x86_64-msvc\libtcod.dll` to the top level source directory (for clarity this is where `build.bat` is). 7. Copy `build\libtcod-1.7.0-x86_64-msvc\SDL2.dll` to the top level source directory. 8. Copy `build\x64\Debug\exe_libtcod\Incursion.exe` to the top level source directory. 9. Open a command prompt in the top-level source directory. 10. Run `Incursion.exe -compile`. Observe the module compiles. 11. Delete `Incursion.exe` from the top-level source directory as it will get stale. 12. Now you should be able to debug and play Incursion in Visual Studio. C:\Data\R\git\incursion-roguelike\build\x64\Debug\exe_libtcod
…heck in `flex.exe` for ease of rebuilding the resource compiler lexer and grammar. Added more build instructions.
…2/x64. Fetches libtcod 1.7.0 dependencies and extracts in correct locations.
Apply a standard format to the entire file Update setup-msbuild action to v2 Compile and upload mod file during debug build Upload fully packaged releases
Clean up msbuild workflow
Always initialize Rect variables Prefer member initializer lists Return full Rect from methods since returning a reference to an internal static variable is a disaster waiting to happen. Mark relevant methods and parameters as const
Update parameter and variable names
This define breaks the standard C++11 array template and must be removed. No code relies on this macro.
Includes `<cstdint>` and updates typedefs to point to it so that I don't need to go around the entire source and update every single type. Replace `long` with `int` in any place where the type is used for a plain value. This clarifies that nothing is broken here. Generally use `int` as the "don't care" type. `int` always means `int32_t` on modern compilers on all platforms. This is convention. `unsigned long` becomes `uint32_t`. `printf` function which was converting pointers to int to print them now uses `%p` instead to print the pointer directly. `EffectInfo::Source` is a suspicious union of pointer and enum types. I used `intptr_t` here for comparisons to prevent truncation. Did not touch `long` in preprocessor or tokenizer sources.
Refactor Rect struct
The class was causing minor warnings and on closer inspection it appears it wasn't being used at all. Blame doesn't go back far enough for me to investigate what this was meant for.
Remove array redefinition
Switch to standard types, replace long types
…ting a String to the embedded buffer in VS2022 where it was in VS2105.
Remove dead code: Dictionary and Parser class
Fixes x64 releases
Bundle mod based on architecture
Looking up the standard docs, 0x00 characters have the same results as 0x01. I can't find any way to explain or justify these macros.
Remove modified character checking functions
…ows top-level or docs directory content to be hosted. 'website' directory was moved to a separate repo which is now hosted on incursion-roguelike.net.
Was causing assertions to fail on negative numbers. A classic case of: that code should be safe to remove, oh it turns out it was there for a reason. Added the functions back with new names so that they don't conflict with anything.
… loaded any more.
…event them being in turn given in the line numbers.
The default working directory is now the project root, always. This is simpler and removes the need to copy files around. Any older save files in `build/run` will have to be moved to the project root folder in order to continue them. I've noticed Incursion breaks with relative paths, otherwise I would've used `"."` as the default directory rather than asking `_getcwd`.
No longer need to wonder if I've modified these files correctly. The CI will now verify and fail if generated files are not committed.
Simplify working directory
Verify generated source files
Missing NULL test in the for-loop initializer meant that the `map` pointer was accessed before being tested. I've documented the macro and made it safer to use. It might be possible to remove the `i` parameter since I don't think anything using this macro accesses that parameter. I'm not sure if `map` needs null testing every loop.
|
My policy is that we do not fix these crashes. The fix is hiding the real problem, which is likely corrupt game state because something did not work right. It is better to crash early than let things potentially get more corrupt and compound the problems further and further. People have been throwing things in Incursion for decades. Why is map not set? How can this happen? What does it mean? |
It's something I'd like to know too. In that case I should probably replace this with more robust code instead. It's just that the current |
|
I am very pedantic about things being better for the players, so feel free to point out if you think I am overly so. Every change we make introduces uncertainty and we should only make them if we are sure we improving things for players is my bar that either of us needs to meet. I find myself asking when a map would ever be completely empty. This feels to me like the real problem. Something has corrupted things before this point. MapIterate is doing us a favour by crashing and showing us. |
|
Well from a player perspective not crashing would be the far better option. For devs, at least having It seems that Thing's missing map pointers are not entirely unusual in this codebase, there are other places which test for this specifically before invoking incursion-roguelike/src/Social.cpp Lines 2440 to 2441 in 490994c In fact |
|
If things are this broken that a maps do not have anything in them, then why would players want them to get worse? |
|
You may have misread the issue. Maps still have stuff in them, but Thing's do not have a reference to the map at various times. This just tells A lot of the code follows outdated C conventions, this is the C solution to this type of problem. Also I just realized that |
|
We need to identify why something broken is involved in game logic. If we change the systems which fine under normal circumstances to hide the problem, then we are not helping the player or developers. Find out how a thing comes to have no map or a map with no things in it or whatever, and find out why it is involved broken like this in game logic. We are not going to be patching MapIterate to work around the core issue. That some broken thing or map is left in place. |
rmtew
force-pushed
the
master
branch
2 times, most recently
from
66243c5 to
ab1053f
Compare
2 participants
Missing NULL test in the for-loop initializer meant that the
mappointer was accessed before being tested. It was easier to fix this than figure out why the Player's map was set to NULL.I've documented the macro and made it safer to use. It might be possible to remove the
iparameter since I don't think anything using this macro accesses that parameter inside the loop body. I'm not sure ifmapneeds null testing every loop. This is the safest I can make it without editing the callers.Apparently this crash was caused by an NPC throwing something at the player. When the code reached
player.isHostileToPartyOfthe players local map pointer was NULL.