Merge pull request #360 from avaraline/ubsan

fixes based on ubsan output

Makefile

@@ -22,6 +22,14 @@ INCFLAGS := $(addprefix -I, $(SRC_DIRS))
 CPPFLAGS := ${CPPFLAGS}
 CPPFLAGS += $(INCFLAGS) -MMD -MP -DNANOGUI_GLAD -g -Wall
 
+# Compile with clang UBSAN
+ifeq ($(AVARA_UBSAN), TRUE)
+LD = clang++
+CPPFLAGS += -fsanitize=undefined -g -fno-omit-frame-pointer
+LDFLAGS += -fsanitize=undefined -g
+endif
+
+# Compile with extra warnings
 ifeq ($(AVARA_WARNINGS), TRUE)
 CPPFLAGS += -pedantic -Wextra -Wcast-align -Wcast-qual -Wctor-dtor-privacy
 CPPFLAGS += -Wdisabled-optimization -Wformat=2 -Winit-self -Wmissing-declarations

src/audio/CBasicSound.h

@@ -33,11 +33,11 @@ class CBasicSound : public CDirectObject {
     int32_t loopCount[2];
     Sample *sampleData;
 
-    int32_t squareAcc[2]; //	Distance squared
-    Fixed dSquare; //	Distance squared as a Fixed.
-    Fixed distance; //	Distance (square root of squareAcc).
-    Fixed relPos[3]; //	Distance as a vector
-    Fixed balance;
+    int32_t squareAcc[2] = {0, 0};//	Distance squared
+    Fixed dSquare = 0; //	Distance squared as a Fixed.
+    Fixed distance = 0; //	Distance (square root of squareAcc).
+    Fixed relPos[3] = {0, 0, 0}; //	Distance as a vector
+    Fixed balance = 0;
 
     SoundLink *motionLink; //	Location information.
     SoundLink *controlLink; //	For control, when motionLink is not good enough.

src/game/CGlowActors.cpp

@@ -68,7 +68,7 @@ void CGlowActors::FrameAction() {
 
     // both the server and client update the FRandSeed here, if they get out of sync
     // then this number will be different and will be noticed in CNetManager::AutoLatencyControl
-    Fixed locsum = location[0] + location[1] + location[2];
-    FRandSeed += locsum;
+    uint32_t locsum = location[0] + location[1] + location[2];
+    UpdateFRandSeed(locsum);
     // SDL_Log("frameNumber = %u, FRandSeed = %10d, locsum = %8d, Actor = %s", gCurrentGame->frameNumber, (Fixed)FRandSeed, locsum, typeid(*this).name());
 }

src/game/CGoody.cpp

@@ -199,7 +199,8 @@ void CGoody::FrameAction() {
         sleepTimer = frequency;
 
     // the goody heading can make a difference in determing a collision with a Hector
-    FRandSeed += heading;
+    // FRandSeed += heading;
+    UpdateFRandSeed((uint32_t)heading);
     // SDL_Log("fn = %ld, goody=%ld: heading = %8d, FRandSeed = %10d\n",
     //         itsGame->frameNumber, ident, heading, (Fixed)FRandSeed);
 }

src/game/CRealMovers.h

@@ -13,8 +13,8 @@
 class CRealMovers : public CGlowActors {
 public:
     Fixed baseMass;
-    Vector speed;
-    Vector dSpeed; // Track delta velocity when Hector gets hit
+    Vector speed = {0, 0, 0, 0}; // Track speed when Hector gets hit
+    Vector dSpeed = {0, 0, 0, 0}; // Track delta velocity when Hector gets hit
 
     virtual void IAbstractActor();
     virtual void GetSpeedEstimate(Fixed *theSpeed);

src/game/CTeleporter.cpp

@@ -194,21 +194,22 @@ void CTeleporter::FrameAction() {
 }
 
 void CTeleporter::TeleportPlayer(CAbstractPlayer *thePlayer) {
-    CTeleporter *theActor;
+    CAbstractActor *theActor;
     CTeleporter *thePort;
     unsigned long maxUse = 0xffffFFFF;
 
-    theActor = (CTeleporter *)itsGame->actorList;
+    theActor = (CAbstractActor *)itsGame->actorList;
     thePort = NULL;
 
     while (theActor) {
         if (theActor->maskBits & kTeleportBit) {
-            if (theActor->transportGroup == destGroup && theActor->useCount < maxUse && theActor != this) {
-                maxUse = theActor->useCount;
-                thePort = theActor;
+            CTeleporter *teleActor = (CTeleporter *)theActor;
+            if (teleActor->transportGroup == destGroup && teleActor->useCount < maxUse && theActor != this) {
+                maxUse = teleActor->useCount;
+                thePort = teleActor;
             }
         }
-        theActor = (CTeleporter *)theActor->nextActor;
+        theActor = (CAbstractActor *)theActor->nextActor;
     }
 
     if (thePort) {

src/util/FastMat.cpp

@@ -25,6 +25,16 @@ static uint16_t arcTanOneTable[ARCUSTABLESIZE] = {0};
 
 Fixed FRandSeed = 1;
 
+void UpdateFRandSeed(uint32_t value) {
+    if (value != 0) {
+        // Use an unsigned intermediate value to calculate the update to FRandSeed
+        // to make sure we don't do the signed overflow (which is undefined behavior)
+        uint32_t newseed = (uint32_t)FRandSeed + value;
+        // cast back to unsigned, but that's OK, as long as everyone is the same
+        FRandSeed = (Fixed) newseed;
+    }
+}
+
 /* FixMath */
 
 Fixed FixATan2(Fixed x, Fixed y) {

src/util/FastMat.h

@@ -157,3 +157,5 @@ typedef struct {
 
 void pidReset(PidMotion *p);
 float pidUpdate(PidMotion *p, float dt, float current, float target);
+
+void UpdateFRandSeed(uint32_t value);

vendor/nanogui/desccombobox.cpp

@@ -35,9 +35,9 @@ DescComboBox::DescComboBox(Widget *parent, const std::vector<std::string> &items
 void DescComboBox::setSelectedIndex(int idx) {
     if (mItemsShort.empty())
         return;
-    const std::vector<Widget *> &children = mButtonWrapper->children();
-    ((Button *) children[mSelectedIndex])->setPushed(false);
-    ((Button *) children[idx])->setPushed(true);
+    //const std::vector<Widget *> &children = mButtonWrapper->children();
+    //((Button *) children[mSelectedIndex])->setPushed(false);
+    //((Button *) children[idx])->setPushed(true);
     mSelectedIndex = idx;
     //setCaption(mItemsShort[idx]);
     setCaption(mItems[idx]);