Out Of Bounds acces in multiplayersetupgroup.cc

[Klaus Halfmann]

When analysing #1730204 with http://clang.llvm.org/docs/AddressSanitizer.html
I found that here is an out of bounds array access in src/wui/multiplayersetupgroup.cc:242

MultiPlayerPlayerGroup subscribes for Notifications for NoteGameSettings but
does no check for the number of slots in the GameSettingsProvider.players.

We actually create more MultiPlayerPlayerGroup instances then we need for the curent Map
(as the map may be changed, later ?)

As this is read acccess I am not sure how bad the effect is in the end.
I will put some hackish bugfix into by local branch for #1730204
and continue to fork of other bugs.

[Klaus Halfmann (klaus-halfmann)]

Found this in bzr8493[bug_1730204-crash].

clang --version
clang version 5.0.0 (tags/RELEASE_500/final)
Target: x86_64-apple-darwin17.2.0
Thread model: posix
InstalledDir: /opt/local/libexec/llvm-5.0/bin

In CMakeList.txt I added

if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")
  # See http://clang.llvm.org/docs/AddressSanitizer.html
  wl_add_flag(WL_COMPILE_DIAGNOSTICS "-fsanitize=address")
  wl_add_flag(WL_COMPILE_DIAGNOSTICS "-fno-omit-frame-pointer")
  set (CMAKE_EXE_LINKER_FLAGS "-fsanitize=address" CACHE STRING "Set by widelands CMakeLists.txt" FORCE)

SirVer: I think its worth the effort to have is as a kind of "standard" tooling.
Slowdon is aroud factor 2 but they claim they _never_ have false postives.

[Klaus Halfmann (klaus-halfmann)]

Fix would be:

   subscriber_ =
      Notifications::subscribe<NoteGameSettings>([this](const NoteGameSettings& note) {
- switch (note.action) {
- case NoteGameSettings::Action::kMap:
- // We don't care about map updates, since we receive enough notifications for the
- // slots.
- break;
- default:
- if (s->settings().players.empty()) {
- // No map/savegame yet
- return;
- }
- if (id_ == note.position ||
- s->settings().players[id_].state == PlayerSettings::State::kShared) {
- update();
- }
+ // We don't care about map updates,
+ // since we receive enough notifications for the slots.
+ if (note.action != NoteGameSettings::Action::kMap) {
+ const std::vector<PlayerSettings>& players = s->settings().players;
+ if (id_ < players.size()) { // cares for empty list, too
+ if (id_ == note.position ||
+ players[id_].state == PlayerSettings::State::kShared) {
+ update();
+ }
+ }
       }
    });

[GunChleoc (gunchleoc)]

I think having the address sanitizer in is an excellent idea - I had been contemplating that too. Is the slowdown in the compile time or during runtime?

Fix looks good to me - can you keep the switch statement though, since it's more efficient than the if statement? So, replace:

    if (players.empty()) {
        // No map/savegame yet
        return;
    }
    if (id_ == note.position ||
            players[id_].state == PlayerSettings::State::kShared) {
        update();
    }

With:

    if (id_ < players.size()) { // Takes care of the empty list, too
        if (id_ == note.position ||
                players[id_].state == PlayerSettings::State::kShared) {
            update();
        }
    }

[Klaus Halfmann (klaus-halfmann)]

Hello Gun, I intend to play a complete game (as training for the tournament)
an try all dialogs I can think of. But Some other Issue eats up my Time once again.
I then wil piece by kece move over the fixed from my current branch.

I can share this branch, if you like. But it may contain som ugly quickhacks,
to get things going

[GunChleoc (gunchleoc)]

Sharing the branch can't hurt - it will give you a free backup, and branches can always be deleted if they end up not being used.