Cataclysm-dda: Reloading makeshift arc welder crashes game

[Stephen2]

Describe the bug

Reloading a newly created "Makeshift arc welder" is crashing my game hard (segfault)

Steps To Reproduce

Haven't tried to reproduce from scratch, but can attach save file.

1. Load save file
2. Note that Makeshift arc welder is wielded
3. Move: SW, W, W, W
4. Press "r" to reload wielded welder
5. Pick any battery

CRASH

Expected behavior

Expect to be able to reload arc welder

Screenshots

n/a

Versions and configuration

• OS: Linux
  
  
  
  • OS Version:
  
  
• Game Version: 0.E-4045-g350a9a9860 [64-bit]
• Graphics Version: Tiles
• Game Language: System language []
• Mods loaded: [
  Dark Days Ahead [dda],
  Disable NPC Needs [no_npc_food],
  DinoMod [DinoMod],
  SpeedyDex [speedydex],
  Stats Through Kills [stats_through_kills],
  Stats Through Skills [StatsThroughSkills]
  ]

Additional context

debug.log
crash.log
save.zip <-- ZIP is actually a 7z file, Github no like that file extension so :shrug:

Built from 4a20b0aa83

[KorGgenT]

you didn't have a battery, so i spawned one in...

[andrei8l]

This issue was introduced by https://github.com/CleverRaven/Cataclysm-DDA/pull/41917/commits/26d6741696de4d566fc42a9c1c3945fa2f34e698#diff-d13ed72411aea98b2ed21e2cb17387ebR2044: it uses an unsafe memory access at lines 2044-2045, then std::moves the data and invalidates the pointers at line 2049 which leads to undefined behavior. Reverting that commit (to use cached variables) fixes the issue on my machine.

[Alex-Folts]

Confirmed on VERSION: 0.E-4042-gcd7577a

New world, new player, spawn welder, battery compartment mod, 2 car battery; install battery compartment mod into welder, reload welder -> chose battery - crash.
Similar on existing save, sometimes first battery installs without crash, but unload welder and try to load another battery - almost guaranted will crash the game.

[Ichthysdekilt]

Can confirm on 10836 as well, as it also happened on reloading a non-makeshift welder.
crash.log
debug.log

It also crashes on load of that save now ever since crashing from reloading.

[hastarl]

I experienced the same crash with an acetylene torch, a food processor, and an H&K MP5. Though the important thing (I think) is that the crash only happens when the magazine is on the ground when reloading. If I have the welding tank, the battery, or the MP5 magazine in my inventory, then it reloads as expected.

This is build 10837 with new world, new character and no mods.

[Alex-Folts]

Though the important thing (I think) is that the crash only happens when the magazine is on the ground when reloading. If I have the welding tank, the battery, or the MP5 magazine in my inventory, then it reloads as expected.

I managed to reload welder while it and battery was on floor without crash, but before had crash when on floor, also had crash when magazine in inventory, when magazine or welder in hands also crash. Crash unpredicatble but chance is decent.

[EvgenijM86]

I have encountered similar crash with soldering iron. The crash does not happen 100% of the time. I reloaded my game, repeated the attempt and for some reason it did not crash this time.

[Ktmi]

Just happened to me as well, reloading on 10837 caused a crash.

[ZaricaDemoni]

Almost every item i can reloaded causes this, and given two new posts regarding reloading causing segment faults, I think its safe to say this one is defiantly confirmed and may want to rename this issue as well as its clear anything that can be reloaded pretty much is causing a segment fault.

Edited to include 2 crash-logs and a debug. one crash-log was for reloading a 20 round stanag magazine, this is repeatable but not guaranteed every time to trigger so i lost track of what i was testing for the second item, it was using a light plutonium battery so most likely a flashlight, game boy, or mp3 player i had in the pile of testing items.

Removed second edit because it was already covered by someone in this list of comments I missed.
Segment Faults on Reloads.zip

[Alex-Folts]

This issue was introduced by 26d6741#diff-d13ed72411aea98b2ed21e2cb17387ebR2044: it uses an unsafe memory access at lines 2044-2045, then std::moves the data and invalidates the pointers at line 2049 which leads to undefined behavior. Reverting that commit (to use cached variables) fixes the issue on my machine.

After reading code over 10 times i think i start getting what you mean.

item &ammo = *act->targets[ 1 ];
and this:
if( !reloadable.reload( *p, std::move( act->targets[ 1 ] ), qty ) ) {
make ammo invalid
and then later he trying to acess ammo in few places
if( ammo.is_filthy() ) {
and here
if( reloadable.has_flag( flag_RELOAD_ONE ) && !ammo.has_flag( flag_SPEEDLOADER ) ) {

so to fix it need to revert changes made to ammo, get back this 2 constants:
const bool is_speedloader = ammo.has_flag( flag_SPEEDLOADER ); const bool ammo_is_filthy = ammo.is_filthy();
and use them instead of ammo.*

[anothersimulacrum]

Or potentially just remove the std::move - what is it doing of value other than invalidating the reference?

[andrei8l]

@Alex-Folts yup looks like you got it. Sorry I wasn't clearer. In my case, I just did git revert 26d6741696de4d566fc42a9c1c3945fa2f34e698 :grin:

[meladath]

I can confirm reverting the above commit removed the crash, I would recommend people do that until this is fixed as it essentially makes the game unplayable for long periods of time.

[Qrox]

Or potentially just remove the std::move - what is it doing of value other than invalidating the reference?

I think the std::move is only invalidating the item location. What really causes ammo to be invalidated is the call to reload.