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.

Sign up for GitHub

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

Jump to bottom

tests: clear missions when clearing avatar #71213

Merged
merged 2 commits into from
Jan 25, 2024
Merged

tests: clear missions when clearing avatar #71213

merged 2 commits into from
Jan 25, 2024

Conversation

andrei8l
Copy link
Contributor

Summary

None

Purpose of change

  1. eoc/math_parser: port time handling to math #70996 exposed a bad condition in EOC_FACTION_SUCCESSION_IMMEDIATE that made it not trigger properly
  2. That EOC exposed a bug in tests code - missions aren't cleared from avatar and leave stale mission type pointers behind

These two together lead to a segfault when tests are ran in a particular order. See this and this.

Describe the solution

  1. Clear missions when clearing avatar
  2. Add a has_var() check to EOC_FACTION_SUCCESSION_IMMEDIATE

Describe alternatives you've considered

N/A

Testing

./cata_test Wield_time_test,widget_value_strings,text_widgets,number_widgets_with_color,graph_widgets,graph_widgets_with_color,widgets_showing_avatar_stats_with_color_for_normal_value,widget_showing_character_fatigue_status,widgets_showing_avatar_health_with_color_for_normal_value,widgets_showing_body_temperature_and_delta,widgets_showing_avatar_stamina,widgets_showing_avatar_weight,widgets_showing_avatar_attributes,widgets_showing_activity_level,widgets_showing_move_counter_and_mode,thirst_and_hunger_widgets,widgets_showing_movement_cost,widgets_showing_Sun_and_Moon_position,widget_showing_body_part_status_text,compact_bodypart_status_widgets_+_legend,outer_armor_widget,radiation_badge_widget,moon_and_lighting_widgets,compass_widget,layout_widgets_in_columns,widgets_showing_weather_conditions,multi-line_overmap_text_widget,Custom_widget_height_and_multiline_formatting,Dynamic_height_for_multiline_widgets,Widget_alignment

doesn't crash

Windows and LTO jobs must succeed for this one.

Additional context

ASan log for the crash, since the builds from CI are not cooperating 
==1071937==ERROR: AddressSanitizer: stack-use-after-return on address 0x74ca31b6d5a0 at pc 0x55d3a6037b3b bp 0x7ffd8466f750 sp 0x7ffd8466f748
READ of size 8 at 0x74ca31b6d5a0 thread T0
    #0 0x55d3a6037b3a in mission::mission_id() const /usr/src/cataclysm-dda-git/src/build_asan/../cataclysm-dda-git/src/mission.cpp:827:9
    #1 0x55d3a6e1f5d6 in talk_effect_fun_t::set_remove_active_mission(JsonObject const&, std::__1::basic_string_view<char, std::__1::char_traits<char>>)::$_0::operator()(dialogue const&) const /usr/src/cataclysm-dda-git/src/build_asan/../cataclysm-dda-git/src/npctalk.cpp:4500:26
    #2 0x55d3a6e1f5d6 in decltype(std::declval<talk_effect_fun_t::set_remove_active_mission(JsonObject const&, std::__1::basic_string_view<char, std::__1::char_traits<char>>)::$_0&>()(std::declval<dialogue&>())) std::__1::__invoke[abi:v160006]<talk_effect_fun_t::set_remove_active_mission(JsonObject const&, std::__1::basic_string_view<char, std::__1::char_traits<char>>)::$_0&, dialogue&>(talk_effect_fun_t::set_remove_active_mission(JsonObject const&, std::__1::basic_string_view<char, std::__1::char_traits<char>>)::$_0&, dialogue&) /usr/bin/../include/c++/v1/__functional/invoke.h:394:23
    #3 0x55d3a6e1f5d6 in void std::__1::__invoke_void_return_wrapper<void, true>::__call<talk_effect_fun_t::set_remove_active_mission(JsonObject const&, std::__1::basic_string_view<char, std::__1::char_traits<char>>)::$_0&, dialogue&>(talk_effect_fun_t::set_remove_active_mission(JsonObject const&, std::__1::basic_string_view<char, std::__1::char_traits<char>>)::$_0&, dialogue&) /usr/bin/../include/c++/v1/__functional/invoke.h:487:9
    #4 0x55d3a6e1f5d6 in std::__1::__function::__alloc_func<talk_effect_fun_t::set_remove_active_mission(JsonObject const&, std::__1::basic_string_view<char, std::__1::char_traits<char>>)::$_0, std::__1::allocator<talk_effect_fun_t::set_remove_active_mission(JsonObject const&, std::__1::basic_string_view<char, std::__1::char_traits<char>>)::$_0>, void (dialogue&)>::operator()[abi:v160006](dialogue&) /usr/bin/../include/c++/v1/__functional/function.h:185:16
    #5 0x55d3a6e1f5d6 in std::__1::__function::__func<talk_effect_fun_t::set_remove_active_mission(JsonObject const&, std::__1::basic_string_view<char, std::__1::char_traits<char>>)::$_0, std::__1::allocator<talk_effect_fun_t::set_remove_active_mission(JsonObject const&, std::__1::basic_string_view<char, std::__1::char_traits<char>>)::$_0>, void (dialogue&)>::operator()(dialogue&) /usr/bin/../include/c++/v1/__functional/function.h:356:12
    #6 0x55d3a6d0357b in std::__1::function<void (dialogue&)>::operator()(dialogue&) const /usr/bin/../include/c++/v1/__functional/function.h:1156:12
    #7 0x55d3a6d0357b in talk_effect_fun_t::operator()(dialogue&) const /usr/src/cataclysm-dda-git/src/build_asan/../cataclysm-dda-git/src/dialogue_helpers.h:170:20
    #8 0x55d3a6d0357b in talk_effect_t::apply(dialogue&) const /usr/src/cataclysm-dda-git/src/build_asan/../cataclysm-dda-git/src/npctalk.cpp:6321:13
    #9 0x55d3a38cff67 in effect_on_condition::activate(dialogue&) const /usr/src/cataclysm-dda-git/src/build_asan/../cataclysm-dda-git/src/effect_on_condition.cpp:308:21
    #10 0x55d3a38d5f10 in eoc_events::notify(cata::event const&, std::__1::unique_ptr<talker, std::__1::default_delete<talker>>, std::__1::unique_ptr<talker, std::__1::default_delete<talker>>) /usr/src/cataclysm-dda-git/src/build_asan/../cataclysm-dda-git/src/effect_on_condition.cpp:557:13
    #11 0x55d3a38d3dba in eoc_events::notify(cata::event const&) /usr/src/cataclysm-dda-git/src/build_asan/../cataclysm-dda-git/src/effect_on_condition.cpp:499:5
    #12 0x55d3a39582dc in event_bus::send(cata::event const&) const /usr/src/cataclysm-dda-git/src/build_asan/../cataclysm-dda-git/src/event_bus.cpp:73:12
    #13 0x55d3a330037c in void event_bus::send<(event_type)9, character_id, int_id<body_part_type> const&>(character_id&&, int_id<body_part_type> const&) const /usr/src/cataclysm-dda-git/src/build_asan/../cataclysm-dda-git/src/event_bus.h:30:13
    #14 0x55d3a330037c in Creature::set_part_hp_cur(int_id<body_part_type> const&, int) /usr/src/cataclysm-dda-git/src/build_asan/../cataclysm-dda-git/src/creature.cpp:2335:25
    #15 0x55d3a2760fcd in Character::set_part_hp_cur(int_id<body_part_type> const&, int) /usr/src/cataclysm-dda-git/src/build_asan/../cataclysm-dda-git/src/character.cpp:1919:15
    #16 0x55d3a19abbb4 in ____C_A_T_C_H____T_E_S_T____127() /usr/src/cataclysm-dda-git/src/build_asan/../cataclysm-dda-git/tests/widget_test.cpp:2439:13
    #17 0x55d3a11aa2c3 in Catch::TestCase::invoke() const /usr/src/cataclysm-dda-git/src/build_asan/../cataclysm-dda-git/tests/catch/catch.hpp:14137:15
    #18 0x55d3a11aa2c3 in Catch::RunContext::invokeActiveTestCase() /usr/src/cataclysm-dda-git/src/build_asan/../cataclysm-dda-git/tests/catch/catch.hpp:12997:27
    #19 0x55d3a11a1fbd in Catch::RunContext::runCurrentTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>&) /usr/src/cataclysm-dda-git/src/build_asan/../cataclysm-dda-git/tests/catch/catch.hpp:12970:17
    #20 0x55d3a119eea2 in Catch::RunContext::runTest(Catch::TestCase const&) /usr/src/cataclysm-dda-git/src/build_asan/../cataclysm-dda-git/tests/catch/catch.hpp:12731:13
    #21 0x55d3a11b666a in Catch::(anonymous namespace)::TestGroup::execute() /usr/src/cataclysm-dda-git/src/build_asan/../cataclysm-dda-git/tests/catch/catch.hpp:13324:45
    #22 0x55d3a11b666a in Catch::Session::runInternal() /usr/src/cataclysm-dda-git/src/build_asan/../cataclysm-dda-git/tests/catch/catch.hpp:13530:39
    #23 0x55d3a11b3704 in Catch::Session::run() /usr/src/cataclysm-dda-git/src/build_asan/../cataclysm-dda-git/tests/catch/catch.hpp:13486:24
    #24 0x55d3a1215c45 in main /usr/src/cataclysm-dda-git/src/build_asan/../cataclysm-dda-git/tests/test_main.cpp:412:26
    #25 0x74ca34f9bccf  (/usr/lib/libc.so.6+0x27ccf) (BuildId: 8bfe03f6bf9b6a6e2591babd0bbc266837d8f658)
    #26 0x74ca34f9bd89 in __libc_start_main (/usr/lib/libc.so.6+0x27d89) (BuildId: 8bfe03f6bf9b6a6e2591babd0bbc266837d8f658)
    #27 0x55d39e751d14 in _start (/usr/src/cataclysm-dda-git/src/build_asan/cata_test+0xba53d14) (BuildId: 3f8b18ebb84a5de5f98a0f4feb87fd01abf5e11d)

Address 0x74ca31b6d5a0 is located in stack of thread T0 at offset 1440 in frame
    #0 0x55d3a8dac7ef in widget::layout(avatar const&, unsigned int, int, bool) /usr/src/cataclysm-dda-git/src/build_asan/../cataclysm-dda-git/src/widget.cpp:1698

  This frame has 37 object(s):
    [32, 33) 'ref.tmp.i2425'
    [48, 56) 'ref.tmp.i2234'
    [80, 88) 'ref.tmp.i2177'
    [112, 120) 'ref.tmp.i2174'
    [144, 152) 'ref.tmp.i1502'
    [176, 976) 'cur_child.i.i' (line 1731)
    [1104, 1128) 'ref.tmp.i'
    [1168, 1184) '__guard.i.i'
    [1200, 1204) 'max_width.addr'
    [1216, 1240) 'ret' (line 1699)
    [1280, 1304) 'wgts' (line 1701)
    [1344, 1368) 'sep' (line 1705)
    [1408, 2208) 'cur_child' (line 1709) <== Memory access at offset 1440 is inside this variable
    [2336, 2360) 'ref.tmp' (line 1710)
    [2400, 2424) 'ref.tmp63' (line 1710)
    [2464, 2488) 'cols' (line 1740)
    [2528, 2552) 'widths' (line 1741)
    [2592, 2596) 'total_width' (line 1742)
    [2608, 2632) 'debug_widths' (line 1743)
    [2672, 3472) 'cur_child198' (line 1746)
    [3600, 3604) 'cur_width' (line 1747)
    [3616, 3640) 'ref.tmp269' (line 1769)
    [3680, 3704) 'ref.tmp296' (line 1775)
    [3744, 3768) 'txt' (line 1780)
    [3808, 3832) 'ref.tmp351' (line 1783)
    [3872, 3896) 'sep382' (line 1788)
    [3936, 3960) 'line' (line 1792)
    [4000, 4024) 'ref.tmp407' (line 1797)
    [4064, 4088) 'ref.tmp453' (line 1805)
    [4128, 4152) 'ref.tmp478' (line 1811)
    [4192, 4216) 'shown' (line 1820)
    [4256, 4280) 'ref.tmp570' (line 1826)
    [4320, 4344) 'ref.tmp611' (line 1835)
    [4384, 4408) 'ref.tmp612' (line 1835)
    [4448, 4512) 'ref.tmp621' (line 1835)
    [4544, 4568) 'ref.tmp689' (line 1844)
    [4608, 4672) 'ref.tmp693' (line 1844)
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-use-after-return /usr/src/cataclysm-dda-git/src/build_asan/../cataclysm-dda-git/src/mission.cpp:827:9 in mission::mission_id() const
Shadow bytes around the buggy address:
  0x74ca31b6d300: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x74ca31b6d380: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x74ca31b6d400: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x74ca31b6d480: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x74ca31b6d500: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
=>0x74ca31b6d580: f5 f5 f5 f5[f5]f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x74ca31b6d600: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x74ca31b6d680: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x74ca31b6d700: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x74ca31b6d780: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x74ca31b6d800: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==1071937==ABORTING

EOC_FACTION_SUCCESSION_IMMEDIATE had a bad condition before because var_name in time_since_var was silently excluded from shimming so that condition was basically interpreted as time_now < time_between_succession. @RenechCDDA can you please double check that EOC chain.

@github-actions github-actions bot added [JSON] Changes (can be) made in JSON Missions Quests and missions Code: Tests Measurement, self-control, statistics, balancing. [C++] Changes (can be) made in C++. Previously named `Code` astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Jan 24, 2024
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 24, 2024
@Maleclypse Maleclypse merged commit ec27a86 into CleverRaven:master Jan 25, 2024
26 checks passed
@andrei8l andrei8l deleted the tests-mission branch January 25, 2024 04:38
@RenechCDDA
Copy link
Member

Additional context

EOC_FACTION_SUCCESSION_IMMEDIATE had a bad condition before because var_name in time_since_var was silently excluded from shimming so that condition was basically interpreted as time_now < time_between_succession. @RenechCDDA can you please double check that EOC chain.

Yeah it looks non-functional now, I'm... not sure you migrated it properly in andrei8l@94a546d ?

It appears that { "math": [ "time_since(timer_time_of_last_succession)", "<", "time_between_succession" ] } (no u_ prefix) functions, whereas the existing { "math": [ "time_since(u_timer_time_of_last_succession)", "<", "time_between_succession" ] } does not. (I tested by stripping out the has_var condition you added in this PR just to eliminate possibilities)

I've confirmed that the variable is in the character space and not a globalvar, so the syntax you added in this PR (u_timer_time_of_last_succession) would be correct, the issue would be in the previous PR.

@andrei8l
Copy link
Contributor Author

It's only set as u_timer_time_of_last_succession so that's the correct scope and name for it. If you strip out has_var() and u_timer_time_of_last_succession is not defined (yet), then time_since() returns -1 so that condition is always true if the var is not defined.

@RenechCDDA
Copy link
Member

RenechCDDA commented Jan 25, 2024 •
edited
Loading

Yes, you're right. Thank you for holding my hand here on and on the development discord. We should be good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` Code: Tests Measurement, self-control, statistics, balancing. [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions Missions Quests and missions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andrei8l @RenechCDDA @Maleclypse