CleverRaven / Cataclysm-DDA

Cataclysm - Dark Days Ahead. A turn-based survival game set in a post-apocalyptic world.
http://cataclysmdda.org
Other
10.62k stars 4.17k forks source link

Smart lamp causes segfault when plugged in and turned on with 0 battery #76319

Open CoroNaut opened 1 month ago

CoroNaut commented 1 month ago

Describe the bug

The smart lamp causes a segfault when plugged in and turned on with 0 power. If the smart lamp has charges in its internal battery regardless of it being plugged in or not, no segfault.

76261 fixed #76245. Unfortunately, this problem was concealed by that one. it looks like "item::getlight_emit()" is the culprit this time.

Attach save file

TESTINGTHREE-trimmed.tar.gz

Steps to reproduce

  1. Spawn storage battery and smart lamp
  2. Turn on smart lamp to get its battery to 0
  3. Plug smart lamp into storage battery
  4. Turning it on causes segfault

Expected behavior

No segfault

Screenshots

No response

Versions and configuration

Additional context

crash.log

PatrikLundell commented 1 month ago

/Confirmed

although I would have preferred if the save had depleted the battery to make it easier to verify... I had some issues trying to actually get rid of the charges (ended up turning on/off repeatedly to shave one charge off each activation).

Edit: To debug it, I had to compile it with debug mode, and to be able to use a breakpoint I had to turn the smart lamp off after draining it, then plug it into the network, place breakpoint, and then turn the lamp on. Not turning the lamp off causes constant breakpoint triggers, probably due to display frame calls.

Blows up at item.cpp operation item::getlight_emit() line const ammotype &loaded_ammo = ammo_data()->ammo->type; item.cpp operation item::ammo_data line return !contents.empty() ? contents.first_ammo().ammo_data() : nullptr; gets called, which returns a null pointer as the contents of the "magazine" is missing.

item::getlight_emit checks whether "ammo" is absent, but does so including linked "ammo", i.e. from the linked battery. The following code then proceeds to use the item's internal "ammo" exclusively, blowing up when it's absent.

There are (at least) two ways to deal with this: A. Change the if( ammo_remaining( true ) == 0 ) { line to pass "false" into the call to ignore the linked battery. That would get around the blowing up thing, but would also mean you'd need a charged battery to get the item to work even when plugged in. B. Change the code for dimming the light when the battery runs out to:

  1. Handle the absence of access to local "ammo" (i.e. not blow up when the internal "ammo" is absent). 2a. Ignore dimming effects if the item is plugged in and has sufficient juice (internal + external); or 2b. Find another way to get at the capacity of the empty battery and use that to apply dimming when the combined internal and external juice available is less than 20% of the internal battery capacity. I don't feel qualified to make the required decisions.
CoroNaut commented 1 month ago

In your A solution, it is definitely reasonable to only allow the light when it has a charge. Think of a laptop, for example. The laptop uses charge, and when plugged in, it simply charges the battery.

Dimming the light would be nice, but it's a smart lamp. It could be smart lamp lore to turn off at any hint that the battery couldn't support it, so it wouldn't damage itself.

GuardianDll commented 1 month ago

Second of Patrik suggestion would be the best one