Merge pull request #92 from Astera-org/mick/ub-adventure

.github/workflows/python.yml

@@ -59,7 +59,7 @@ jobs:
           CC: clang-14
           CXX: clang++-14
           CMAKE_BUILD_SERVER: FALSE
-          CMAKE_FLAGS: -DUSE_SDL2=ON
+          CMAKE_FLAGS: -DUSE_SDL2=ON -DSANITIZER=ubsan
 
       - name: Run pytest
         run: |

.gitignore

@@ -97,6 +97,7 @@ doc/mkdocs/mkdocs.yml
 
 ## Build files
 build/
+/build-*/
 CMakeFiles
 Makefile
 cmake_install.cmake

CMakeLists.txt

@@ -78,6 +78,24 @@ message(STATUS "RUN_IN_PLACE: " ${RUN_IN_PLACE})
 
 set(WARN_ALL TRUE CACHE BOOL "Enable -Wall for Release build")
 
+# Define a variable to control sanitizers
+set(SANITIZER "none" CACHE STRING "Choose sanitizer: none, ubsan, asan")
+
+# Print the chosen sanitizer
+message(STATUS "SANITIZER: ${SANITIZER}")
+
+if(SANITIZER STREQUAL "ubsan")
+    add_compile_options(-fsanitize=undefined -fno-omit-frame-pointer)
+    add_link_options(-fsanitize=undefined)
+elseif(SANITIZER STREQUAL "asan")
+    add_compile_options(-fsanitize=address -fno-omit-frame-pointer)
+    add_link_options(-fsanitize=address)
+elseif(SANITIZER STREQUAL "none")
+    message(STATUS "No sanitizer enabled")
+else()
+    message(FATAL_ERROR "Invalid SANITIZER value: ${SANITIZER}. Choose from: none, ubsan, asan")
+endif()
+
 if(NOT CMAKE_BUILD_TYPE)
 	# Default to release
 	set(CMAKE_BUILD_TYPE Release CACHE STRING "Build type: Debug or Release" FORCE)

irr/src/CIrrDeviceSDL.cpp

@@ -713,7 +713,12 @@ bool CIrrDeviceSDL::run()
 
 	while (!Close && wrap_PollEvent(&SDL_event)) {
 		// os::Printer::log("event: ", core::stringc((int)SDL_event.type).c_str(),   ELL_INFORMATION);	// just for debugging
+#pragma GCC diagnostic push
+#ifndef __clang__
+#pragma GCC diagnostic ignored "-Wclass-memaccess"
+#endif
 		memset(&irrevent, 0, sizeof(irrevent));
+#pragma GCC diagnostic pop
 
 		switch (SDL_event.type) {
 		case SDL_MOUSEMOTION: {

mick.bash

@@ -0,0 +1,37 @@
+#!/usr/bin/env bash
+
+set -euo pipefail
+
+cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null
+
+build_with_compiler() {
+	export CC="$1"
+    export CXX="$2"
+
+    local build_dir="build-$CXX-$3"
+
+    cmake -B "$build_dir" -S . \
+		-DCMAKE_C_COMPILER="$1" \
+		-DCMAKE_CXX_COMPILER="$2" \
+		-DCMAKE_FIND_FRAMEWORK=LAST \
+		-DRUN_IN_PLACE=TRUE \
+		-DENABLE_SOUND=FALSE \
+		-DENABLE_GETTEXT=TRUE \
+		-GNinja \
+		-DUSE_SDL2=ON \
+		-DSANITIZER="$3" \
+		-DCMAKE_CXX_LINK_FLAGS="-fuse-ld=mold" \
+		-DCMAKE_BUILD_TYPE=Debug \
+		-DCMAKE_EXPORT_COMPILE_COMMANDS=1 \
+		-DCMAKE_COLOR_DIAGNOSTICS=ON \
+		-DBUILD_SERVER=ON
+
+    cmake --build "$build_dir" -j "$(nproc)"
+}
+
+build_with_compiler gcc-14 g++-14 none
+# build_with_compiler gcc-14 g++-14 ubsan
+# build_with_compiler gcc-14 g++-14 asan
+build_with_compiler clang-18 clang++-18 none
+build_with_compiler clang-18 clang++-18 ubsan
+build_with_compiler clang-18 clang++-18 asan

setupMinetestAsteraInfra.md

@@ -26,10 +26,12 @@ cmake -B build -S . \
 	-DENABLE_GETTEXT=TRUE \
 	-GNinja \
 	-DUSE_SDL2=ON \
-	-DCMAKE_CXX_FLAGS="-fuse-ld=mold" \
+	-DSANITIZER="none" \
+	-DCMAKE_CXX_LINK_FLAGS="-fuse-ld=mold" \
 	-DCMAKE_BUILD_TYPE=Debug \
-	-DCMAKE_EXPORT_COMPILE_COMMANDS=1 && \
-	cmake --build build
+	-DCMAKE_EXPORT_COMPILE_COMMANDS=1 \
+	-DCMAKE_COLOR_DIAGNOSTICS=ON && \
+	cmake --build build -j $(nproc)
 
 pushd minetest-gymnasium && pytest .; popd
 ```

src/CMakeLists.txt

@@ -855,7 +855,7 @@ if(MSVC)
 	# Flags that cannot be shared between cl and clang-cl
 	# https://clang.llvm.org/docs/UsersManual.html#clang-cl
 	if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
-		set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fuse-ld=lld")
+		set(DCMAKE_CXX_LINK_FLAGS "${DCMAKE_CXX_LINK_FLAGS} -fuse-ld=lld")
 
 		# Disable pragma-pack warning
 		set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -Wno-pragma-pack")
@@ -866,8 +866,9 @@ if(MSVC)
 	endif()
 else()
 	# GCC or compatible compilers such as Clang
-	set(WARNING_FLAGS "-Wall -Wextra")
+	set(WARNING_FLAGS "-Wall -Wextra -Werror")
 	set(WARNING_FLAGS "${WARNING_FLAGS} -Wno-unused-parameter")
+
 	if(WARN_ALL)
 		set(RELEASE_WARNING_FLAGS "${WARNING_FLAGS}")
 	else()

src/client/inputhandler.h

@@ -25,6 +25,11 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 #include <list>
 #include "keycode.h"
 
+#pragma GCC diagnostic push
+#if __clang__
+#pragma GCC diagnostic ignored "-Winconsistent-missing-override"
+#endif
+
 class InputHandler;
 
 enum class PointerType {
@@ -413,3 +418,5 @@ class RandomInputHandler final : public InputHandler
 	float joystickSpeed;
 	float joystickDirection;
 };
+
+#pragma GCC diagnostic pop

src/client/remoteinputhandler.cpp

@@ -7,17 +7,30 @@
 #include "client/content_cao.h"
 #include <cassert>
 #include <mutex>
+#include <sstream>
+#include <stdexcept>
 #include <string>
 #include <capnp/message.h>
 #include <capnp/serialize.h>
 #include <capnp/rpc.h>
 #include <kj/array.h>
 #include <vector>
-
+#include <limits>
+
+#pragma GCC diagnostic push
+#if __clang__
+#pragma GCC diagnostic error "-Weverything"
+#pragma GCC diagnostic ignored "-Wc++98-compat"
+#pragma GCC diagnostic ignored "-Wctad-maybe-unsupported"
+#if (__clang_major__ > 18) || (__clang_major__ == 18 && __clang_minor__ >= 1)
+#pragma GCC diagnostic ignored "-Wunsafe-buffer-usage"
+#endif
+#endif
+#pragma GCC diagnostic ignored "-Wpadded"
 
 namespace detail {
 
-kj::Promise<void> MinetestImpl::init(InitContext context) {
+kj::Promise<void> MinetestImpl::init(InitContext) {
   std::unique_lock<std::mutex> lock(m_chan->m_action_mutex);
   m_chan->m_did_init = true;
   m_chan->m_action_cv.notify_one();
@@ -50,6 +63,18 @@ kj::Promise<void> MinetestImpl::step(StepContext context) {
 
 } // namespace detail
 
+namespace {
+  s32 to_s32(u32 value) {
+    // TODO: when c++-20 use https://en.cppreference.com/w/cpp/utility/in_range
+    if (value > std::numeric_limits<s32>::max()) {
+      auto message = std::stringstream{};
+      message << "u32 " << value << " does not fit in s32";
+      throw std::range_error{message.str()};
+    }
+    return static_cast<s32>(value);
+  }
+}
+
 RemoteInputHandler::RemoteInputHandler(const std::string &endpoint,
                                        RenderingEngine *rendering_engine,
                                        MyEventReceiver *receiver)
@@ -74,7 +99,7 @@ RemoteInputHandler::RemoteInputHandler(const std::string &endpoint,
     server.listen(*listener).wait(capnp_io_context.waitScope);
   }).detach();
   m_chan.m_action_cv.wait(lock, [this] { return m_chan.m_did_init; });
-};
+}
 
 void RemoteInputHandler::step(float dtime) {
   // skip first loop, because we don't have an observation yet
@@ -97,7 +122,10 @@ void RemoteInputHandler::step(float dtime) {
       KeyPress key_code = keycache.key[static_cast<int>(keyEvent)];
       new_key_is_down.set(key_code);
     }
-    mouse_movement = v2f(m_chan.m_action->getMouseDx(), m_chan.m_action->getMouseDy());
+    mouse_movement = v2f(
+      static_cast<float>(m_chan.m_action->getMouseDx()),
+      static_cast<float>(m_chan.m_action->getMouseDy())
+    );
 
     m_chan.m_action = nullptr;
     m_chan.m_action_cv.notify_one();
@@ -143,7 +171,7 @@ void RemoteInputHandler::step(float dtime) {
   }
 
   m_should_send_observation = true;
-};
+}
 
 void RemoteInputHandler::step_post_render() {
   if (!m_should_send_observation) {
@@ -158,8 +186,8 @@ void RemoteInputHandler::step_post_render() {
   {
     irr::video::IImage *image = m_rendering_engine->get_video_driver()->createScreenShot(video::ECF_R8G8B8);
     auto builder = obs_builder.initImage();
-    builder.setWidth(image->getDimension().Width);
-    builder.setHeight(image->getDimension().Height);
+    builder.setWidth(to_s32(image->getDimension().Width));
+    builder.setHeight(to_s32(image->getDimension().Height));
     builder.setData(capnp::Data::Reader(reinterpret_cast<const uint8_t *>(image->getData()), image->getImageDataSizeInBytes()));
     image->drop();
   }
@@ -168,10 +196,10 @@ void RemoteInputHandler::step_post_render() {
     const auto lock_guard = std::lock_guard { m_player->exposeTheMutex() };
     const auto& hud_elements = m_player->exposeTheHud();
     constexpr auto is_text = [](const HudElement * element) noexcept { return element->type == HUD_ELEM_TEXT; };
-    const auto text_count = std::count_if(hud_elements.begin(), hud_elements.end(), is_text);
+    const auto text_count = std::count_if(hud_elements.cbegin(), hud_elements.cend(), is_text);
 
     auto builder = obs_builder.initHudElements();
-    auto entries = builder.initEntries(text_count);
+    auto entries = builder.initEntries(static_cast<unsigned int>(text_count));
     auto entry_it = entries.begin();
     for (const auto& hud_element : hud_elements) {
       if (!is_text(hud_element)) continue;
@@ -181,19 +209,34 @@ void RemoteInputHandler::step_post_render() {
     }
   }
 
-  obs_builder.setPlayerHealth(m_player->hp);
-  obs_builder.setPlayerHealthMax(m_player->getCAO()->getProperties().hp_max);
-  obs_builder.setPlayerBreath(m_player->getBreath());
-  obs_builder.setPlayerBreathMax(m_player->getCAO()->getProperties().breath_max);
-  obs_builder.setPlayerIsDead(m_player->isDead());
-
   {
     auto server_lock = std::lock_guard { gServer->getEnvMutex() };
+
     auto remote_player = gServer->getEnv().getPlayer(m_player->getName());
-    assert(remote_player != nullptr);
-    const auto& player_meta = remote_player->getPlayerSAO()->getMeta().getStrings();
+    if (remote_player == nullptr) {
+      throw std::runtime_error("remote_player is null");
+    }
+
+    auto remote_player_sao = remote_player->getPlayerSAO();
+    if (remote_player_sao == nullptr) {
+      throw std::runtime_error("remote_player sao is null");
+    }
+
+    auto remote_player_props = remote_player_sao->accessObjectProperties();
+    if (remote_player_props == nullptr) {
+      throw std::runtime_error("remote_player object properties is null");
+    }
+
+    // We are retrieving these from the server because `m_player-getCAO()` can return a null pointer.
+    obs_builder.setPlayerHealth(remote_player_sao->getHP());
+    obs_builder.setPlayerHealthMax(remote_player_props->hp_max);
+    obs_builder.setPlayerBreath(remote_player_sao->getBreath());
+    obs_builder.setPlayerBreathMax(remote_player_props->breath_max);
+    obs_builder.setPlayerIsDead(remote_player_sao->isDead());
+
+    const auto& player_meta = remote_player_sao->getMeta().getStrings();
     auto builder = obs_builder.initPlayerMetadata();
-    auto entries = builder.initEntries(player_meta.size());
+    auto entries = builder.initEntries(static_cast<unsigned int>(player_meta.size()));
     auto entry_it = entries.begin();
     for (const auto& [key, value] : player_meta) {
       entry_it->setKey(key);
@@ -207,3 +250,5 @@ void RemoteInputHandler::step_post_render() {
   m_chan.m_obs_msg_builder.reset(builder_buffer);
   m_chan.m_obs_cv.notify_one();
 }
+
+#pragma GCC diagnostic pop

src/client/remoteinputhandler.h

@@ -1,4 +1,5 @@
 #pragma once
+
 #include "client/renderingengine.h"
 #include "gui/mainmenumanager.h"
 #include "inputhandler.h"
@@ -15,6 +16,16 @@
 #include <capnp/serialize-packed.h>
 #include <kj/async-io.h>
 
+#pragma GCC diagnostic push
+#if __clang__
+#pragma GCC diagnostic error "-Weverything"
+#pragma GCC diagnostic ignored "-Wc++98-compat"
+#if (__clang_major__ > 18) || (__clang_major__ == 18 && __clang_minor__ >= 1)
+#pragma GCC diagnostic ignored "-Wunsafe-buffer-usage"
+#endif
+#endif
+#pragma GCC diagnostic ignored "-Wpadded"
+
 namespace detail {
 struct Channel {
   std::condition_variable m_action_cv;
@@ -29,7 +40,7 @@ struct Channel {
   Channel() {}
 };
 
-class MinetestImpl : public Minetest::Server {
+class MinetestImpl final : public Minetest::Server {
 public:
   MinetestImpl(Channel *chan) : m_chan(chan) {}
   kj::Promise<void> init(InitContext context) override;
@@ -73,7 +84,7 @@ class RemoteInputHandler : public InputHandler {
   }
   virtual float getJoystickDirection() override {
     auto axes = getJoystickAxes();
-    return (axes.X == 0 && axes.Y == 0) ? 0.0 : atan2((double) axes.X, (double) axes.Y);
+    return (axes.X == 0 && axes.Y == 0) ? 0.0 : std::atan2(static_cast<float>(axes.X), static_cast<float>(axes.Y));
   }
   virtual v2s32 getMousePos() override { return m_mouse_pos; }
   virtual void setMousePos(s32 x, s32 y) override { m_mouse_pos = v2s32(x, y); }
@@ -109,11 +120,11 @@ class RemoteInputHandler : public InputHandler {
   // Returns a vector with 2 elements in the set (-1, 0, 1) representing the joystick axes.
   virtual v2s32 getJoystickAxes() {
     return v2s32(
-      (int) m_key_is_down[keycache.key[KeyType::RIGHT]] -
-        (int) m_key_is_down[keycache.key[KeyType::LEFT]]
+      static_cast<int>(m_key_is_down[keycache.key[KeyType::RIGHT]]) -
+        static_cast<int>(m_key_is_down[keycache.key[KeyType::LEFT]])
       ,
-      (int) m_key_is_down[keycache.key[KeyType::FORWARD]] -
-        (int) m_key_is_down[keycache.key[KeyType::BACKWARD]]
+      static_cast<int>(m_key_is_down[keycache.key[KeyType::FORWARD]]) -
+        static_cast<int>(m_key_is_down[keycache.key[KeyType::BACKWARD]])
     );
   }
 
@@ -151,3 +162,5 @@ class RemoteInputHandler : public InputHandler {
 
   bool m_should_send_observation = false;
 };
+
+#pragma GCC diagnostic pop

src/filesys.cpp

@@ -528,7 +528,9 @@ bool CopyFileContents(const std::string &source, const std::string &target)
 	sourcefile.reset(fopen(source.c_str(), "rb"));
 	targetfile.reset(fopen(target.c_str(), "wb"));
 
+#ifdef __linux__
 fallback:
+#endif
 
 	if (!sourcefile) {
 		errorstream << source << ": can't open for reading: "

src/gui/guiTable.cpp

@@ -1121,8 +1121,13 @@ void GUITable::sendTableEvent(s32 column, bool doubleclick)
 	m_sel_column = column;
 	m_sel_doubleclick = doubleclick;
 	if (Parent) {
+#pragma GCC diagnostic push
+#ifndef __clang__
+#pragma GCC diagnostic ignored "-Wclass-memaccess"
+#endif
 		SEvent e;
 		memset(&e, 0, sizeof e);
+#pragma GCC diagnostic pop
 		e.EventType = EET_GUI_EVENT;
 		e.GUIEvent.Caller = this;
 		e.GUIEvent.Element = 0;