Issue907

Title [SIGSEGV] Crash when handling inventory
Priority release-blocker Status rejected
Assigned To Keywords
Linked issues Watchers

Submitted on 2017-11-29 19h54 by jesusalva, last changed by fluzz.

Messages
Author: jesusalva Date: 2017-11-29   19h54
Try rapidly switching Laser Power Pack (or Plasma Canister) with a Laser Pistol
on your inventory. Game crashes. (I managed to reproduce, therefore, reporting.
The console output follows)

print_trace:  Obtained 9 stack frames.
print_trace:  Obtaining symbols now done.
./src(print_trace+0x12) [0x454852]
/lib/x86_64-linux-gnu/libc.so.6(+0x354b0) [0x7fc3a1cf74b0]
./src(show_current_text_banner+0x86) [0x430826]
./src(show_texts_and_banner+0x2b) [0x43188b]
./src(AssembleCombatPicture+0x723) [0x46f2d3]
./src(Game+0x4c) [0x44acfc]
./src(main+0x14a) [0x4188ea]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0) [0x7fc3a1ce2830]
./src(_start+0x29) [0x418989]

print_trace():  received SIGSEGV!

---------------------------------------------------------------------------------
Termination of freedroidRPG initiated... Thank you for playing freedroidRPG.
Author: jesusalva Date: 2017-12-24   17h32
Argh, now switching 100 Plasma Grenades with 1 Industrial Coolant.
Earlier I've got another crash related to that, but I forgot to take note.

<module-list>

print_trace:  Obtained 9 stack frames.
print_trace:  Obtaining symbols now done.
./src(print_trace+0x12) [0x454852]
/lib/x86_64-linux-gnu/libc.so.6(+0x354b0) [0x7f22717ec4b0]
./src(show_current_text_banner+0x86) [0x430826]
./src(show_texts_and_banner+0x2b) [0x43188b]
./src(AssembleCombatPicture+0x723) [0x46f2d3]
./src(Game+0x4c) [0x44acfc]
./src(main+0x14a) [0x4188ea]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0) [0x7f22717d7830]
./src(_start+0x29) [0x418989]

print_trace():  received SIGSEGV!

---------------------------------------------------------------------------------
Termination of freedroidRPG initiated... Thank you for playing freedroidRPG.

Raising priority as this bug can be reproduced multiple times with different
items, and it provokes a crash.
Author: jesusalva Date: 2018-01-24   14h49
Raising priority to release-blocker.
Should be fixed ASAP.

Bug does not happen only with ammo, but with all sort of items.
@sydney also reported same crash.
It is impossible to release another version without this crash being corrected.
Author: jesusalva Date: 2018-01-24   21h09
Some testing with printf's was done in order to understand better what is
causing the crash.

Here is a (formated) terminal output:

> show_current_text_banner() called. Plasma Transistor → Plasma Transistor
> show_current_text_banner() called. Plasma Transistor → Entropy Inverter
> show_current_text_banner() called. Entropy Inverter
> print_trace:  Obtained 9 stack frames.

Where → is a call to prepare_text_window_content.


On that specific case, the error happens at line 290 of hud.c: Commenting it
will provoke weird, unexpected behavior when switching items (eg. REQUERIMENTS
NOT MET being displayed with non-equippable items, game freezes for a couple of
seconds, etc.) but crash seems to be averted.

That is not a solution. This is all information I managed to gather to help 
solving this issue.
Author: jesusalva Date: 2018-01-25   03h48
My speculation:

I guess the bug is a simple memory mis-procedure: When you switch items, the
memory buffer(?) which holds the previous item data must be freed, and
re-allocated for the new item data. But this procedure is not done, instead, we
try to add the new item data to the previous item data (causing a mess, and the
crash)
We only free the data when we drop the item, or unhover it. As switching items
is neither of those - we do not free the data, and jump straight to writing the
new item data (which doesn't crashes immediately, but corrupts all data,
crashing a little after, when processing the new autostr)
Author: tuxcomp Date: 2018-02-28   23h29
This issue is caused by the "Automatic Small-Items Collection - RR 2370"
patch ( https://rb.freedroid.org/r/2370 ) and only present when
automatic-collection is enabled.

reproduce: 
 - enable auto-collect (game options)
 - then open inventory, pick up a normal item (e.g. some weapon)
 - try place the weapon to a slot of an auto-collected item (e.g. a potion).

when swapping items:  drop_held_item_to_inventory () ->  MakeHeldFloorItemOutOf
()  "item_held_in_hand" now points to a free entry in CURLEVEL()->ItemList[] ;
and not to an item in Me.Inventory[].

handle_automatic_item_collection () comes around and checks the Level's
ItemList, auto-collects the item (add to Me.inventory), and calls 
raw_move_picked_up_item_to_entry() which clears the item in the Level-list  and
sets type = -1.  However, item_held_in_hand is not changed.

Now item_held_in_hand is not NULL and item_held_in_hand->type = -1 ; this causes
a crash later in src/hud.c when looking up 
item_specs_get_name(item_held_in_hand->type)
Author: tuxcomp Date: 2018-03-01   00h29
a workaround on top of diff at RR 2370:

```
diff --git a/src/influ.c b/src/influ.c
index 9a6b1291d..a5107a4cb 100644
--- a/src/influ.c
+++ b/src/influ.c
@@ -1649,6 +1649,10 @@ static void handle_automatic_item_collection()
                        can_pickup = 1;
                }
 
+               if (item_held_in_hand == &obj_lvl->ItemList[tmp]) {
+                       can_pickup = 0;
+               }
+
                if (can_pickup) {
                        give_item(&obj_lvl->ItemList[tmp]);
                }
```

But really, the patch RR2370 should probably not be merged: 
get_floor_item_index_under_me() is called on ever call to move_tux().
get_floor_item_index_under_me() iterates over all visible levels and over all
items per level and compares the position; it also calls
update_virtual_position() every time.. 

Even though it the function checks for items in a given radius it returns at
most one (and iteratively collects items if Tux is moving slow enough at high FPS).

While the feature is very convenient, the current implementation (RR2370) is
lacking, sadly.
Author: fluzz Date: 2018-03-20   17h12
Jesusalva, can you confirm that this crash only happens if RR2370 is applied ?
If so, this is definitely not a release blocker, given that RR2370 is not (yet)
planned to be included in the next release !
History
Date User Action Args
2018-03-20 17:12:22fluzzsetmessages: + msg3553
2018-03-01 00:43:09jesusalvalinkissue905 linked
2018-03-01 00:42:12jesusalvasetstatus: open -> rejected
2018-03-01 00:29:47tuxcompsetmessages: + msg3543
2018-02-28 23:29:12tuxcompsetmessages: + msg3542
2018-01-25 03:48:18jesusalvasetmessages: + msg3538
2018-01-24 21:09:46jesusalvasetmessages: + msg3537
2018-01-24 14:49:51jesusalvasetpriority: critical -> release-blocker
messages: + msg3536
2017-12-24 17:39:48jesusalvalinkissue749 linked
2017-12-24 17:32:44jesusalvasetpriority: important -> critical
messages: + msg3523
2017-11-29 19:54:00jesusalvacreate