Deallocated Meory usage from GameMainMenuSaveGame::entry_selected

[Klaus Halfmann]

http://clang.llvm.org/docs/AddressSanitizer.html.

shows me that some deallcoated memotry is used here:

void GameMainMenuSaveGame::entry_selected() {
 ok_.set_enabled(load_or_save_.table().selections().size() == 1);
 load_or_save_.delete_button()->set_enabled(load_or_save_.has_selection());
 if (load_or_save_.has_selection()) {
  const SavegameData& gamedata = *load_or_save_.entry_selected();
-> filename_editbox_.set_text(
-> FileSystem::filename_without_ext(gamedata.filename.c_str()));
 }
}

will now take apart that expression to find the details

[SirVer (sirver)]

Klaus, you are doing invaluable work lately with your ASAN investigations! Thanks!

[Klaus Halfmann (klaus-halfmann)]

OK I refactored this to:

if (load_or_save_.has_selection()) {
  const SavegameData& gamedata = *load_or_save_.entry_selected();
> std::string gamefileName = gamedata.filename; // provoke copy to Trigger Adress sanitizer
  const char* const filename = gamedata.filename.c_str();
  std::string without_ext = FileSystem::filename_without_ext(filename);
  filename_editbox_.set_text(without_ext);
}

So that SavegameData was already released?
No idea why we convert between std:string and char* but that an other issue.

OK, lets find out where this SavegameData comes from.

SirVer: Sorry I have only this week of vacation,
hunting bugs with this tool can be funny first and daunting, later.
Maybe I should calim another Nickname "The Sanitizer" :-)

[GunChleoc (gunchleoc)]

Jukka already provided a fix for this one, which got merged only yesterday

http://bazaar.launchpad.net/~widelands-dev/widelands/trunk/revision/8495#src/wui/load_or_save_game.cc

[Klaus Halfmann (klaus-halfmann)]

OK I will merge trunk, commit my branch and check again.

I this fails I will add some "poor mans delete check":
in the DTor (e.g. ~SavegameData()} I will set some
extra variable to some "magic" value and assert to
it.

Gun: we may add this as a standard feature so some
of the UI Base classes, what do you think.

[Klaus Halfmann (klaus-halfmann)]

Sorry, I still get that crash, will try fo find the reason now.

[Klaus Halfmann (klaus-halfmann)]

Mhh, adding a DTor ~SavegameData(), makes the compile complain,
e.g.

bug_1730204-crash/src/wui/gamedetails.h:67:5:
error: definition of implicit copy constructor for 'SavegameData' is
      deprecated because it has a user-declared destructor [-Werror,-Wdeprecated]
    ~SavegameData();

bug_1730204-crash/src/wui/load_or_save_game.cc:430:16:
note: in instantiation of member function
      'std::__1::vector<SavegameData, std::__1::allocator<SavegameData> >::push_back' requested here
    games_data_.push_back(gamedata);

So on the one hand SavegameData shall be a stupid struct only,
but is used as full object.
I am not really sure about the ownership of this data.
It is deleted via unique_ptr.reset(). But maybe this is not the way unique_ptr must be used.

I will not be able to focus on this Problem today, sorry

[GunChleoc (gunchleoc)]

NP - I am using this bug right now to test adding ASAN to our compile script. I have also added an option to choose the compiler. Currently waiting to see if it works with GCC as well :)

[Klaus Halfmann (klaus-halfmann)]

This got worse with bzr8497[run-asan] now.

I now sees this already when opening a normal network game:

==67333==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x612000f35e60 at pc 0x00010b031099 bp 0x7ffee6285130 sp 0x7ffee6285128
READ of size 4 at 0x612000f35e60 thread T0
    #0 0x10b031098 in MultiPlayerPlayerGroup::MultiPlayerPlayerGroup(UI::Panel*, int, int, unsigned char, GameSettingsProvider*, NetworkPlayerSettingsBackend*)::'lambda'(No

allocated by thread T0 here:
std::__1::allocator<PlayerSettings>&>::__split_buffer(unsigned long, unsigned long, std::__1::allocator<PlayerSettings>&) (/Users/klaus/develop/widelands-repo/run-asan/./widelands:x86_64+0x100dee9f5)
    #2 0x10a754a9c in std::__1::__split_buffer<PlayerSettings, std::__1::allocator<PlayerSettings>&>::__split_buffer(unsigned long, unsigned long, std::__1::allocator<PlayerSettings>&) (/Users/klaus/develop/widelands-repo/run-asan/./widelands:x86_64+0x100deba9c)
    #3 0x10a7544eb in std::__1::vector<PlayerSettings, std::__1::allocator<PlayerSettings> >::__append(unsigned long) (/Users/klaus/develop/widelands-repo/run-asan/./widelands:x86_64+0x100deb4eb)
    #4 0x10a725e69 in std::__1::vector<PlayerSettings, std::__1::allocator<PlayerSettings> >::resize(unsigned long) (/Users/klaus/develop/widelands-repo/run-asan/./widelands:x86_64+0x100dbce69)
    #5 0x10a7809e4 in GameHost::set_map(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, unsigned int, bool) (/Users/klaus/develop/widelands-repo/run-asan/./widelands:x86_64+0x100e179e4)
    #6 0x10a7b7903 in HostGameSettingsProvider::set_map(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, unsigned int, bool) (/Users/klaus/develop/widelands-repo/run-asan/./widelands:x86_64+0x100e4e903)
    #7 0x10ab36120 in FullscreenMenuLaunchMPG::select_saved_game() (/Users/klaus/develop/widelands-repo/run-asan/./widelands:x86_64+0x1011cd120)

[Klaus Halfmann (klaus-halfmann)]

With the upcoming ASA change this will make networking Games impossible :\

[GunChleoc (gunchleoc)]

Just run compile.sh -a, or use the CMake option -DOPTION_ASAN=OFF, or use a release build. I added that option on purpose, so nobody will have to compile with ASAN.

[GunChleoc (gunchleoc)]

The second issue has now also been reported as a separate bug, so I'll mark this one as fix committed for the original bzr reported here.

https://bugs.launchpad.net/widelands/+bug/1734534

[GunChleoc (gunchleoc)]

Fixed in build20-rc1