Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crash when comparing daypack and military rucksack #34046

Closed
nornagon opened this issue Sep 15, 2019 · 8 comments · Fixed by #36606
Closed

Crash when comparing daypack and military rucksack #34046

nornagon opened this issue Sep 15, 2019 · 8 comments · Fixed by #36606
Labels
<Crash / Freeze> Fatal bug that results in hangs or crashes. Inventory / AIM / Zones Inventory, Advanced Inventory Management or Zones Items / Item Actions / Item Qualities Items and how they work and interact (S2 - Confirmed) Bug that's been confirmed to exist
Projects
Milestone

Comments

@nornagon
Copy link
Contributor

Game version: 0.D-7603-g1b96ad13c5-dirty

(but I didn't change anything related to encumbrance in my local build)

Operating system: macOS 10.13.6

Tiles or curses: Tiles

Mods active:

Expected behavior

Don't crash when comparing items

Actual behavior

SIGSEGV with

0   libsystem_kernel.dylib        	0x00007fff55715b66 __pthread_kill + 10
1   libsystem_pthread.dylib       	0x00007fff558e0080 pthread_kill + 333
2   libsystem_c.dylib             	0x00007fff556711ae abort + 127
3   cataclysm-tiles               	0x000000010fb88643 signal_handler(int) + 83
4   libsystem_platform.dylib      	0x00007fff558d3f5a _sigtramp + 26
5   ???                           	0x0000000111636e00 0 + 4586696192
6   cataclysm-tiles               	0x000000010fe7a533 item::is_null() const + 99
7   cataclysm-tiles               	0x000000010fe7b55c item::volume(bool) const + 28
8   cataclysm-tiles               	0x000000010feba53b item::get_encumber(Character const&) const + 75
9   cataclysm-tiles               	0x000000010fe7dfde item::info(std::__1::vector<iteminfo, std::__1::allocator<iteminfo> >&, iteminfo_query const*, int) const + 782
10  cataclysm-tiles               	0x000000010fe7dc81 item::info(bool, std::__1::vector<iteminfo, std::__1::allocator<iteminfo> >&) const + 49
11  cataclysm-tiles               	0x000000010fd95477 game_menus::inv::compare(player&, cata::optional<tripoint> const&) + 1991
12  cataclysm-tiles               	0x000000010fdb8cc4 game::handle_action() + 7908
13  cataclysm-tiles               	0x000000010fcedbcf game::do_turn() + 2399
[...]

Steps to reproduce the behavior

In a new world, spawn a military rucksack and a daypack. Wear the military rucksack. Then press I to compare items, and use the search function / to search for "mil", press enter to confirm the search, and press right arrow to select the rucksack you're wearing. Then press / again, search for "day", press enter to confirm the search, then press right arrow to select the daypack. The game will crash before it shows the comparison screen.

This is 100% reproducible for me.

@Night-Pryanik Night-Pryanik added (S1 - Need confirmation) Report waiting on confirmation of reproducibility <Crash / Freeze> Fatal bug that results in hangs or crashes. Items / Item Actions / Item Qualities Items and how they work and interact labels Sep 15, 2019
@ZhilkinSerg ZhilkinSerg pinned this issue Sep 15, 2019
@ZhilkinSerg ZhilkinSerg unpinned this issue Sep 15, 2019
@ZhilkinSerg ZhilkinSerg added (S2 - Confirmed) Bug that's been confirmed to exist and removed (S1 - Need confirmation) Report waiting on confirmation of reproducibility labels Sep 15, 2019
@ZhilkinSerg
Copy link
Contributor

Successfully reproduced crash on Windows with current master:

image

@Andrew-Fall
Copy link
Contributor

Andrew-Fall commented Sep 15, 2019

Caused by inventory_ui.cpp line 1916
image

@ishtatann
Copy link
Contributor

On that inventory compare crash - inventory_column::prepare_paging erases all the inventory_entry's from entries on a filter change. That leaves compared with an invalid pointer. std::vector<inventory_entry > compared should probably be an item (i.e. inventory_entry->locations->front()->get_item() )

@KorGgenT
Copy link
Member

fixed by #36139

0.E Release automation moved this from Fix Proposed to Done Dec 19, 2019
@ZhilkinSerg
Copy link
Contributor

Not fixed yet.

@ZhilkinSerg ZhilkinSerg reopened this Dec 19, 2019
0.E Release automation moved this from Done to Confirmed Dec 19, 2019
@KorGgenT
Copy link
Member

my apologies, misread it

@KorGgenT KorGgenT moved this from Confirmed to Needs re-confirmation in 0.E Release Dec 19, 2019
@I-am-Erk I-am-Erk moved this from Needs re-confirmation to Confirmed in 0.E Release Dec 19, 2019
@ghost
Copy link

ghost commented Dec 19, 2019

confirmed. still happening, on Linux at least.

@ghost
Copy link

ghost commented Dec 19, 2019

can we revive the proposed fix direction in #36139 ?

jbytheway added a commit to jbytheway/Cataclysm-DDA that referenced this issue Jan 1, 2020
Fixes CleverRaven#34046.

Using the search filter causes all the inventory_entry objects to be
recreated, so it's not safe to store a pointer to them.

A previous attempt to solve this (CleverRaven#36139) stored copies of the
inventory_entry objects, but that was also unsafe.

This attempt instead stores just pointers to the underlying items.  This
seems to prevent the crashes.

The behaviour of the window is still not always exactly correct, because
using the filter will violate the invariant that the items in the
compared vector correspond to the selected entries.  I don't see a way
to fix that in general -- sometimes the compared item is filtered out,
and thus you can't possibly select the entry (because it doesn't exist).
We might want to add something that does its best to select the right
entries when the filter changes, but this is a sufficiently niche use
case that I'm not sure it's worth the complexity.  I've settled for just
fixing the crash for now.
jbytheway added a commit to jbytheway/Cataclysm-DDA that referenced this issue Jan 1, 2020
Fixes CleverRaven#34046.

Using the search filter causes all the inventory_entry objects to be
recreated, so it's not safe to store a pointer to them.

A previous attempt to solve this (CleverRaven#36139) stored copies of the
inventory_entry objects, but that was also unsafe.

This attempt instead stores just pointers to the underlying items.  This
seems to prevent the crashes.

The behaviour of the window is still not always exactly correct, because
using the filter will violate the invariant that the items in the
compared vector correspond to the selected entries.  I don't see a way
to fix that in general -- sometimes the compared item is filtered out,
and thus you can't possibly select the entry (because it doesn't exist).
We might want to add something that does its best to select the right
entries when the filter changes, but this is a sufficiently niche use
case that I'm not sure it's worth the complexity.  I've settled for just
fixing the crash for now.
0.E Release automation moved this from Confirmed to Done Jan 2, 2020
kevingranade pushed a commit that referenced this issue Jan 2, 2020
Fixes #34046.

Using the search filter causes all the inventory_entry objects to be
recreated, so it's not safe to store a pointer to them.

A previous attempt to solve this (#36139) stored copies of the
inventory_entry objects, but that was also unsafe.

This attempt instead stores just pointers to the underlying items.  This
seems to prevent the crashes.

The behaviour of the window is still not always exactly correct, because
using the filter will violate the invariant that the items in the
compared vector correspond to the selected entries.  I don't see a way
to fix that in general -- sometimes the compared item is filtered out,
and thus you can't possibly select the entry (because it doesn't exist).
We might want to add something that does its best to select the right
entries when the filter changes, but this is a sufficiently niche use
case that I'm not sure it's worth the complexity.  I've settled for just
fixing the crash for now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
<Crash / Freeze> Fatal bug that results in hangs or crashes. Inventory / AIM / Zones Inventory, Advanced Inventory Management or Zones Items / Item Actions / Item Qualities Items and how they work and interact (S2 - Confirmed) Bug that's been confirmed to exist
Projects
No open projects
0.E Release
  
Done
7 participants