Merge pull request #445 from Kurausukun/ubfix

Port UBFIX Macro and Some Usages from Emerald

include/config.h

@@ -41,6 +41,9 @@
 #ifndef BUGFIX
 #define BUGFIX
 #endif // BUGFIX
+#ifndef UBFIX
+#define UBFIX
+#endif // UBFIX
 #endif // MODERN
 
 #endif // GUARD_CONFIG_H

include/global.h

@@ -73,6 +73,14 @@
 #define abs(x) (((x) < 0) ? -(x) : (x))
 #endif
 
+// Used in cases where division by 0 can occur in the retail version.
+// Avoids invalid opcodes on some emulators, and the otherwise UB.
+#ifdef UBFIX
+#define SAFE_DIV(a, b) ((b) ? (a) / (b) : 0)
+#else
+#define SAFE_DIV(a, b) ((a) / (b))
+#endif
+
 // There are many quirks in the source code which have overarching behavioral differences from
 // a number of other files. For example, diploma.c seems to declare rodata before each use while
 // other files declare out of order and must be at the beginning. There are also a number of

src/agb_flash.c

@@ -127,7 +127,9 @@ void SetReadFlash1(u16 *dest)
     }
 }
 
-void ReadFlash_Core(u8 *src, u8 *dest, u32 size)
+
+// Using volatile here to make sure the flash memory will ONLY be read as bytes, to prevent any compiler optimizations.
+void ReadFlash_Core(vu8 *src, u8 *dest, u32 size)
 {
     while (size-- != 0)
     {
@@ -142,7 +144,7 @@ void ReadFlash(u16 sectorNum, u32 offset, void *dest, u32 size)
     vu16 readFlash_Core_Buffer[0x40];
     vu16 *funcSrc;
     vu16 *funcDest;
-    void (*readFlash_Core)(u8 *, u8 *, u32);
+    void (*readFlash_Core)(vu8 *, u8 *, u32);
 
     REG_WAITCNT = (REG_WAITCNT & ~WAITCNT_SRAM_MASK) | WAITCNT_SRAM_8;
 
@@ -164,7 +166,7 @@ void ReadFlash(u16 sectorNum, u32 offset, void *dest, u32 size)
         i--;
     }
 
-    readFlash_Core = (void (*)(u8 *, u8 *, u32))((s32)readFlash_Core_Buffer + 1);
+    readFlash_Core = (void (*)(vu8 *, u8 *, u32))((s32)readFlash_Core_Buffer + 1);
 
     src = FLASH_BASE + (sectorNum << gFlash->sector.shift) + offset;
 

src/battle_ai_script_commands.c

@@ -1362,7 +1362,12 @@ static void Cmd_if_status_not_in_party(void)
 
         // everytime the status is found, the AI's logic jumps further and further past its intended destination. this results in a broken AI macro and is probably why it is unused.
         if (species != SPECIES_NONE && species != SPECIES_EGG && hp != 0 && status == statusToCompareTo)
+        {
             sAIScriptPtr += 10; // doesnt return?
+            #ifdef UBFIX
+            return;
+            #endif
+        }
     }
     sAIScriptPtr = T1_READ_PTR(sAIScriptPtr + 6);
 }

src/battle_anim_mons.c

@@ -1706,7 +1706,7 @@ void SetBattlerSpriteYOffsetFromOtherYScale(u8 spriteId, u8 otherSpriteId)
 {
     s32 var = 64 - GetBattlerYDeltaFromSpriteId(otherSpriteId) * 2;
     u16 matrix = gSprites[spriteId].oam.matrixNum;
-    s32 var2 = (var << 8) / gOamMatrices[matrix].d;
+    s32 var2 = SAFE_DIV((var << 8), gOamMatrices[matrix].d);
 
     if (var2 > 128)
         var2 = 128;

src/battle_script_commands.c

@@ -3108,7 +3108,7 @@ static void atk23_getexp(void)
             calculatedExp = gBaseStats[gBattleMons[gBattlerFainted].species].expYield * gBattleMons[gBattlerFainted].level / 7;
             if (viaExpShare) // at least one mon is getting exp via exp share
             {
-                *exp = calculatedExp / 2 / viaSentIn;
+                *exp = SAFE_DIV(calculatedExp / 2, viaSentIn);
                 if (*exp == 0)
                     *exp = 1;
                 gExpShareExp = calculatedExp / 2 / viaExpShare;
@@ -3117,7 +3117,7 @@ static void atk23_getexp(void)
             }
             else
             {
-                *exp = calculatedExp / viaSentIn;
+                *exp = SAFE_DIV(calculatedExp, viaSentIn);
                 if (*exp == 0)
                     *exp = 1;
                 gExpShareExp = 0;

src/easy_chat.c

@@ -459,12 +459,17 @@ void InitEasyChatPhrases(void)
             gSaveBlock1Ptr->mail[i].words[j] = EC_WORD_UNDEFINED;
     }
 
+#ifndef UBFIX
     // BUG: This is supposed to clear 64 bits, but this loop is clearing 64 bytes.
     // However, this bug has no resulting effect on gameplay because only the
     // Mauville old man data is corrupted, which is initialized directly after
     // this function is called when starting a new game.
     for (i = 0; i < 64; i++)
         gSaveBlock1Ptr->additionalPhrases[i] = 0;
+#else
+    for (i = 0; i < NELEMS(gSaveBlock1Ptr->additionalPhrases); i++)
+        gSaveBlock1Ptr->additionalPhrases[i] = 0;
+#endif
 }
 
 void EC_ResetMEventProfileMaybe(void)

src/event_object_movement.c

@@ -2561,7 +2561,13 @@ const u8 *GetObjectEventScriptPointerByObjectEventId(u8 objectEventId)
 
 static u16 GetObjectEventFlagIdByLocalIdAndMap(u8 localId, u8 mapNum, u8 mapGroup)
 {
-    return GetObjectEventTemplateByLocalIdAndMap(localId, mapNum, mapGroup)->flagId;
+    struct ObjectEventTemplate *obj = GetObjectEventTemplateByLocalIdAndMap(localId, mapNum, mapGroup);
+#ifdef UBFIX
+    // BUG: The function may return NULL, and attempting to read from NULL may freeze the game using modern compilers.
+    if (obj == NULL)
+        return 0;
+#endif // UBFIX
+    return obj->flagId;
 }
 
 static u16 GetObjectEventFlagIdByObjectEventId(u8 objectEventId)

src/field_effect.c

@@ -3666,8 +3666,10 @@ static void Task_MoveDeoxysRock_Step(u8 taskId)
     case 0:
         data[4] = sprite->pos1.x << 4;
         data[5] = sprite->pos1.y << 4;
-        data[6] = ((data[2] << 4) - data[4]) / data[8];
+
-        data[7] = ((data[3] << 4) - data[5]) / data[8];
+        // UB: Possible divide by zero
+        data[6] = SAFE_DIV(((data[2] << 4) - data[4]), data[8]);
+        data[7] = SAFE_DIV(((data[3] << 4) - data[5]), data[8]);
         data[0]++;
         // fallthrough
     case 1:

src/fieldmap.c

@@ -528,10 +528,14 @@ static bool32 SavedMapViewIsEmpty(void)
     u16 i;
     u32 marker = 0;
 
+#ifndef UBFIX
     // BUG: This loop extends past the bounds of the mapView array. Its size is only 0x100.
     for (i = 0; i < 0x200; i++)
         marker |= gSaveBlock2Ptr->mapView[i];
-
+#else
+    for (i = 0; i < NELEMS(gSaveBlock2Ptr->mapView); i++)
+        marker |= gSaveBlock2Ptr->mapView[i];
+#endif
     if (marker == 0)
         return TRUE;
     else
@@ -746,14 +750,32 @@ struct MapConnection *sub_8059600(u8 direction, s32 x, s32 y)
 {
     s32 count;
     struct MapConnection *connection;
+    const struct MapConnections *connections = gMapHeader.connections;
     s32 i;
-    count = gMapHeader.connections->count;
+    // UB: Multiple possible null dereferences
-    connection = gMapHeader.connections->connections;
+#ifdef UBFIX
+    if (connections != NULL)
+    {
+        count = connections->count;
+        connection = connections->connections;
+        if (connection != NULL)
+        {
+            for (i = 0; i < count; i++, connection++)
+            {
+                if (connection->direction == direction && sub_8059658(direction, x, y, connection) == TRUE)
+                    return connection;
+            }
+        }
+    }
+#else
+    count = connections->count;
+    connection = connections->connections;
     for (i = 0; i < count; i++, connection++)
     {
         if (connection->direction == direction && sub_8059658(direction, x, y, connection) == TRUE)
             return connection;
     }
+#endif
     return NULL;
 
 }

src/pokemon_storage_system_7.c

@@ -473,8 +473,13 @@ static void sub_80957C8(void)
         for (j = sMoveMonsPtr->minRow; j < rowCount; j++)
         {
             struct BoxPokemon *boxMon = GetBoxedMonPtr(boxId, boxPosition);
-
+            // UB: possible null dereference
+#ifdef UBFIX
+            if (boxMon != NULL)
+                sMoveMonsPtr->boxMons[monArrayId] = *boxMon;
+#else
             sMoveMonsPtr->boxMons[monArrayId] = *boxMon;
+#endif
             monArrayId++;
             boxPosition++;
         }

src/sprite.c

@@ -1333,7 +1333,7 @@ void ApplyAffineAnimFrameRelativeAndUpdateMatrix(u8 matrixNum, struct AffineAnim
 s16 ConvertScaleParam(s16 scale)
 {
     s32 val = 0x10000;
-    return val / scale;
+    return SAFE_DIV(val, scale);
 }
 
 void GetAffineAnimFrame(u8 matrixNum, struct Sprite *sprite, struct AffineAnimFrameCmd *frameCmd)

src/union_room.c

@@ -1628,7 +1628,11 @@ static bool32 IsPartnerActivityAcceptable(u32 activity, u32 group)
     if (group == 0xFF)
         return TRUE;
 
+    #ifndef UBFIX
     if (group <= NELEMS(sAcceptedActivityIds)) // UB: <= may access data outside the array
+    #else
+    if (group < NELEMS(sAcceptedActivityIds))
+    #endif
     {
         const u8 *bytes = sAcceptedActivityIds[group];