From 9b341a4d6a721b46baa81ee2f47815b6073f5096 Mon Sep 17 00:00:00 2001 From: Anghelo Carvajal Date: Sat, 24 Jun 2023 04:40:26 -0400 Subject: [PATCH] A few UB fixes (#1272) * Fix OoB in ObjDriftie * Fix OoB in EnHorseLinkChild * Fix negative shift in jpegdecode * more oob fixes * AVOID_UB * clean * huh? * change viint.h macros * objdriftice * ub labelling * review * review * fix z_parameter arrays * u32 cast * missing & --- include/ultra64/viint.h | 16 ++--- src/audio/lib/heap.c | 4 +- src/audio/lib/load.c | 2 + src/code/jpegdecoder.c | 2 +- src/code/z_parameter.c | 65 +++++++++++++------ src/code/z_vimode.c | 8 +-- .../z_en_horse_link_child.c | 10 +-- .../actors/ovl_Obj_Driftice/z_obj_driftice.c | 17 +++-- .../actors/ovl_Obj_Driftice/z_obj_driftice.h | 6 +- tools/disasm/variables.txt | 15 ++--- 10 files changed, 86 insertions(+), 59 deletions(-) diff --git a/include/ultra64/viint.h b/include/ultra64/viint.h index 4fea2259a2..6f61307eb0 100644 --- a/include/ultra64/viint.h +++ b/include/ultra64/viint.h @@ -17,17 +17,17 @@ // For use in initializing OSViMode structures #define BURST(hsync_width, color_width, vsync_width, color_start) \ - (hsync_width | (color_width << 8) | (vsync_width << 16) | (color_start << 20)) -#define WIDTH(v) v -#define VSYNC(v) v -#define HSYNC(duration, leap) (duration | (leap << 16)) -#define LEAP(upper, lower) ((upper << 16) | lower) -#define START(start, end) ((start << 16) | end) + (((u32)(hsync_width) & 0xFF) | (((u32)(color_width) & 0xFF) << 8) | (((u32)(vsync_width) & 0xF) << 16) | (((u32)(color_start) & 0xFFFF) << 20)) +#define WIDTH(v) (v) +#define VSYNC(v) (v) +#define HSYNC(duration, leap) (((u32)(leap) << 16) | ((u32)(duration) & 0xFFFF)) +#define LEAP(upper, lower) (((u32)(upper) << 16) | ((u32)(lower) & 0xFFFF)) +#define START(start, end) (((u32)(start) << 16) | ((u32)(end) & 0xFFFF)) -#define FTOFIX(val, i, f) ((u32)(val * (f32)(1 << f)) & ((1 << (i + f)) - 1)) +#define FTOFIX(val, i, f) ((u32)((val) * (f32)(1 << (f))) & ((1 << ((i) + (f))) - 1)) #define F210(val) FTOFIX(val, 2, 10) -#define SCALE(scaleup, off) (F210((1.0f / (f32)scaleup)) | (F210((f32)off) << 16)) +#define SCALE(scaleup, off) (F210((1.0f / (f32)(scaleup))) | (F210((f32)(off)) << 16)) #define VCURRENT(v) v #define ORIGIN(v) v diff --git a/src/audio/lib/heap.c b/src/audio/lib/heap.c index 7a7cd58071..5d8dfee894 100644 --- a/src/audio/lib/heap.c +++ b/src/audio/lib/heap.c @@ -1109,7 +1109,9 @@ void* AudioHeap_AllocPermanent(s32 tableType, s32 id, size_t size) { gAudioCtx.permanentEntries[index].size = size; //! @bug UB: missing return. "addr" is in v0 at this point, but doing an // explicit return uses an additional register. - // return addr; +#ifdef AVOID_UB + return addr; +#endif } void* AudioHeap_AllocSampleCache(size_t size, s32 sampleBankId, void* sampleAddr, s8 medium, s32 cache) { diff --git a/src/audio/lib/load.c b/src/audio/lib/load.c index af9de1cd1c..34e8dec9c2 100644 --- a/src/audio/lib/load.c +++ b/src/audio/lib/load.c @@ -738,6 +738,7 @@ void* AudioLoad_SyncLoad(s32 tableType, u32 id, s32* didAllocate) { romAddr = table->entries[realId].romAddr; switch (cachePolicy) { case CACHE_LOAD_PERMANENT: + //! @bug UB: triggers an UB because this function is missing a return value. ramAddr = AudioHeap_AllocPermanent(tableType, realId, size); if (ramAddr == NULL) { return ramAddr; @@ -1108,6 +1109,7 @@ void* AudioLoad_AsyncLoadInner(s32 tableType, s32 id, s32 nChunks, s32 retData, switch (cachePolicy) { case CACHE_LOAD_PERMANENT: + //! @bug UB: triggers an UB because this function is missing a return value. ramAddr = AudioHeap_AllocPermanent(tableType, realId, size); if (ramAddr == NULL) { return ramAddr; diff --git a/src/code/jpegdecoder.c b/src/code/jpegdecoder.c index 336e381f9e..e6998adc99 100644 --- a/src/code/jpegdecoder.c +++ b/src/code/jpegdecoder.c @@ -156,7 +156,7 @@ s32 JpegDecoder_ParseNextSymbol(JpegHuffmanTable* hTable, s16* outCoeff, s8* out if (sym) { *outCoeff = JpegDecoder_ReadBits(sym); if (*outCoeff < (1 << (sym - 1))) { - *outCoeff += (-1 << sym) + 1; + *outCoeff += (0xFFFFFFFF << sym) + 1; } } diff --git a/src/code/z_parameter.c b/src/code/z_parameter.c index c6a0862763..8cec0a5b75 100644 --- a/src/code/z_parameter.c +++ b/src/code/z_parameter.c @@ -159,16 +159,33 @@ static Gfx sScreenFillSetupDL[] = { s16 D_801BF9B0 = 0; f32 D_801BF9B4[] = { 100.0f, 109.0f }; -s16 D_801BF9BC[] = { 0x226, 0x2A8, 0x2A8, 0x2A8 }; +s16 D_801BF9BC[] = { + 0x226, // EQUIP_SLOT_B + 0x2A8, // EQUIP_SLOT_C_LEFT + 0x2A8, // EQUIP_SLOT_C_DOWN + 0x2A8, // EQUIP_SLOT_C_RIGHT +}; s16 D_801BF9C4[] = { 0x9E, 0x9B }; s16 D_801BF9C8[] = { 0x17, 0x16 }; f32 D_801BF9CC[] = { -380.0f, -350.0f }; -s16 D_801BF9D4[] = { 0xA7, 0xE3 }; -s16 D_801BF9D8[] = { 0xF9, 0x10F }; -s16 D_801BF9DC[] = { 0x11, 0x12 }; -s16 D_801BF9E0[] = { 0x22, 0x12 }; -s16 D_801BF9E4[] = { 0x23F, 0x26C }; -s16 D_801BF9E8[] = { 0x26C, 0x26C }; +s16 D_801BF9D4[] = { + 0xA7, // EQUIP_SLOT_B + 0xE3, // EQUIP_SLOT_C_LEFT + 0xF9, // EQUIP_SLOT_C_DOWN + 0x10F, // EQUIP_SLOT_C_RIGHT +}; +s16 D_801BF9DC[] = { + 0x11, // EQUIP_SLOT_B + 0x12, // EQUIP_SLOT_C_LEFT + 0x22, // EQUIP_SLOT_C_DOWN + 0x12, // EQUIP_SLOT_C_RIGHT +}; +s16 D_801BF9E4[] = { + 0x23F, // EQUIP_SLOT_B + 0x26C, // EQUIP_SLOT_C_LEFT + 0x26C, // EQUIP_SLOT_C_DOWN + 0x26C, // EQUIP_SLOT_C_RIGHT +}; s16 sFinalHoursClockDigitsRed = 0; s16 sFinalHoursClockFrameEnvRed = 0; @@ -3848,8 +3865,12 @@ void Interface_DrawItemButtons(PlayState* play) { // Remnant of OoT 130, 136, 136, 136, 136, }; - static s16 D_801BFAF4[] = { 0x1D, 0x1B }; - static s16 D_801BFAF8[] = { 0x1B, 0x1B }; + static s16 D_801BFAF4[] = { + 0x1D, // EQUIP_SLOT_B + 0x1B, // EQUIP_SLOT_C_LEFT + 0x1B, // EQUIP_SLOT_C_DOWN + 0x1B, // EQUIP_SLOT_C_RIGHT + }; InterfaceContext* interfaceCtx = &play->interfaceCtx; Player* player = GET_PLAYER(play); PauseContext* pauseCtx = &play->pauseCtx; @@ -3863,21 +3884,27 @@ void Interface_DrawItemButtons(PlayState* play) { gDPSetCombineMode(OVERLAY_DISP++, G_CC_MODULATEIA_PRIM, G_CC_MODULATEIA_PRIM); // B Button Color & Texture - OVERLAY_DISP = Gfx_DrawTexRectIA8_DropShadow(OVERLAY_DISP, gButtonBackgroundTex, 0x20, 0x20, D_801BF9D4[0], - D_801BF9DC[0], D_801BFAF4[0], D_801BFAF4[0], D_801BF9E4[0] * 2, - D_801BF9E4[0] * 2, 100, 255, 120, interfaceCtx->bAlpha); + OVERLAY_DISP = Gfx_DrawTexRectIA8_DropShadow( + OVERLAY_DISP, gButtonBackgroundTex, 0x20, 0x20, D_801BF9D4[EQUIP_SLOT_B], D_801BF9DC[EQUIP_SLOT_B], + D_801BFAF4[EQUIP_SLOT_B], D_801BFAF4[EQUIP_SLOT_B], D_801BF9E4[EQUIP_SLOT_B] * 2, D_801BF9E4[EQUIP_SLOT_B] * 2, + 100, 255, 120, interfaceCtx->bAlpha); gDPPipeSync(OVERLAY_DISP++); // C-Left Button Color & Texture - OVERLAY_DISP = Gfx_DrawRect_DropShadow(OVERLAY_DISP, D_801BF9D4[1], D_801BF9DC[1], D_801BFAF4[1], D_801BFAF4[1], - D_801BF9E4[1] * 2, D_801BF9E4[1] * 2, 255, 240, 0, interfaceCtx->cLeftAlpha); + OVERLAY_DISP = Gfx_DrawRect_DropShadow(OVERLAY_DISP, D_801BF9D4[EQUIP_SLOT_C_LEFT], D_801BF9DC[EQUIP_SLOT_C_LEFT], + D_801BFAF4[EQUIP_SLOT_C_LEFT], D_801BFAF4[EQUIP_SLOT_C_LEFT], + D_801BF9E4[EQUIP_SLOT_C_LEFT] * 2, D_801BF9E4[EQUIP_SLOT_C_LEFT] * 2, 255, + 240, 0, interfaceCtx->cLeftAlpha); // C-Down Button Color & Texture - OVERLAY_DISP = Gfx_DrawRect_DropShadow(OVERLAY_DISP, D_801BF9D8[0], D_801BF9E0[0], D_801BFAF8[0], D_801BFAF8[0], - D_801BF9E4[2] * 2, D_801BF9E4[2] * 2, 255, 240, 0, interfaceCtx->cDownAlpha); + OVERLAY_DISP = Gfx_DrawRect_DropShadow(OVERLAY_DISP, D_801BF9D4[EQUIP_SLOT_C_DOWN], D_801BF9DC[EQUIP_SLOT_C_DOWN], + D_801BFAF4[EQUIP_SLOT_C_DOWN], D_801BFAF4[EQUIP_SLOT_C_DOWN], + D_801BF9E4[EQUIP_SLOT_C_DOWN] * 2, D_801BF9E4[EQUIP_SLOT_C_DOWN] * 2, 255, + 240, 0, interfaceCtx->cDownAlpha); // C-Right Button Color & Texture - OVERLAY_DISP = - Gfx_DrawRect_DropShadow(OVERLAY_DISP, D_801BF9D8[1], D_801BF9E0[1], D_801BFAF8[1], D_801BFAF8[1], - D_801BF9E4[3] * 2, D_801BF9E4[3] * 2, 255, 240, 0, interfaceCtx->cRightAlpha); + OVERLAY_DISP = Gfx_DrawRect_DropShadow(OVERLAY_DISP, D_801BF9D4[EQUIP_SLOT_C_RIGHT], D_801BF9DC[EQUIP_SLOT_C_RIGHT], + D_801BFAF4[EQUIP_SLOT_C_RIGHT], D_801BFAF4[EQUIP_SLOT_C_RIGHT], + D_801BF9E4[EQUIP_SLOT_C_RIGHT] * 2, D_801BF9E4[EQUIP_SLOT_C_RIGHT] * 2, 255, + 240, 0, interfaceCtx->cRightAlpha); if (!IS_PAUSE_STATE_GAMEOVER) { if ((play->pauseCtx.state != PAUSE_STATE_OFF) || (play->pauseCtx.debugEditor != DEBUG_EDITOR_NONE)) { diff --git a/src/code/z_vimode.c b/src/code/z_vimode.c index 2ddd277754..3909c7a05b 100644 --- a/src/code/z_vimode.c +++ b/src/code/z_vimode.c @@ -153,14 +153,14 @@ void ViMode_Configure(OSViMode* viMode, s32 type, s32 tvType, s32 loRes, s32 ant viMode->comRegs.vSync++; if (tvType == OS_TV_MPAL) { viMode->comRegs.hSync += HSYNC(1, 4); - viMode->comRegs.leap += LEAP((u16)-4, (u16)-2); + viMode->comRegs.leap += LEAP(-4, -2); } } else { - viMode->fldRegs[0].vStart += START((u16)-3, (u16)-2); + viMode->fldRegs[0].vStart += START(-3, -2); if (tvType == OS_TV_MPAL) { - viMode->fldRegs[0].vBurst += BURST((u8)-2, (u8)-1, 12, -1); + viMode->fldRegs[0].vBurst += BURST(-2, -1, 12, -1); } else if (tvType == OS_TV_PAL) { - viMode->fldRegs[1].vBurst += BURST((u8)-2, (u8)-1, 2, 0); + viMode->fldRegs[1].vBurst += BURST(-2, -1, 2, 0); } } diff --git a/src/overlays/actors/ovl_En_Horse_Link_Child/z_en_horse_link_child.c b/src/overlays/actors/ovl_En_Horse_Link_Child/z_en_horse_link_child.c index 13812397b8..d867d299f8 100644 --- a/src/overlays/actors/ovl_En_Horse_Link_Child/z_en_horse_link_child.c +++ b/src/overlays/actors/ovl_En_Horse_Link_Child/z_en_horse_link_child.c @@ -39,10 +39,10 @@ ActorInit En_Horse_Link_Child_InitVars = { (ActorFunc)EnHorseLinkChild_Draw, }; -AnimationHeader* D_808DFEC0[] = { &object_horse_link_child_Anim_006D44, &object_horse_link_child_Anim_007468 }; - -AnimationHeader* D_808DFEC8[] = { &object_horse_link_child_Anim_007D50, &object_horse_link_child_Anim_0043AC, - &object_horse_link_child_Anim_002F98 }; +AnimationHeader* D_808DFEC0[] = { + &object_horse_link_child_Anim_006D44, &object_horse_link_child_Anim_007468, &object_horse_link_child_Anim_007D50, + &object_horse_link_child_Anim_0043AC, &object_horse_link_child_Anim_002F98, +}; static ColliderJntSphElementInit sJntSphElementsInit[] = { { @@ -444,7 +444,7 @@ void func_808DF620(EnHorseLinkChild* this, PlayState* play) { if (Math_CosS(sp36) < 0.0f) { this->unk_148 = 2; Animation_Change(&this->skin.skelAnime, D_808DFEC0[this->unk_148], D_808DFF18[this->unk_148], 0.0f, - Animation_GetLastFrame(D_808DFEC8[0]), ANIMMODE_ONCE, -5.0f); + Animation_GetLastFrame(D_808DFEC0[2]), ANIMMODE_ONCE, -5.0f); } else { func_808DF560(this); } diff --git a/src/overlays/actors/ovl_Obj_Driftice/z_obj_driftice.c b/src/overlays/actors/ovl_Obj_Driftice/z_obj_driftice.c index 7d729495a0..13a88568b0 100644 --- a/src/overlays/actors/ovl_Obj_Driftice/z_obj_driftice.c +++ b/src/overlays/actors/ovl_Obj_Driftice/z_obj_driftice.c @@ -163,19 +163,19 @@ void func_80A66930(ObjDrifticeStruct2* arg0, ObjDriftice* this, s16* arg2, s16* temp_f20 = 0.01f; } - Math_StepToS(&arg0->unk_00[1], 1200.0f * temp_f20, 0x64); + Math_StepToS(&arg0->unk_02, 1200.0f * temp_f20, 0x64); phi_s0 = 2500.0f * temp_f20; - Math_StepToS(&arg0->unk_00[3], Math_CosS(arg0->unk_00[2] * 6.5536f) * (120.0f * temp_f20), 0x28); + Math_StepToS(&arg0->unk_06, Math_CosS(arg0->unk_04 * 6.5536f) * (120.0f * temp_f20), 0x28); Math_StepToF(&arg0->unk_08, 1.0f, 0.02f); } else { - Math_StepToS(&arg0->unk_00[1], 0, 0x96); + Math_StepToS(&arg0->unk_02, 0, 0x96); phi_s0 = 0; - Math_StepToS(&arg0->unk_00[3], 20, 7); + Math_StepToS(&arg0->unk_06, 20, 7); Math_StepToF(&arg0->unk_08, 0.0f, 0.02f); } - Math_ScaledStepToS(arg0->unk_00, this->dyna.actor.yawTowardsPlayer, arg0->unk_00[1]); - *arg3 = arg0->unk_00[0]; - Math_ScaledStepToS(&arg0->unk_00[2], phi_s0, arg0->unk_00[3]); + Math_ScaledStepToS(&arg0->unk_00, this->dyna.actor.yawTowardsPlayer, arg0->unk_02); + *arg3 = arg0->unk_00; + Math_ScaledStepToS(&arg0->unk_04, phi_s0, arg0->unk_06); for (i = 0; i < 2; i++) { temp_s0 = &arg0->unk_0C[i]; @@ -195,8 +195,7 @@ void func_80A66930(ObjDrifticeStruct2* arg0, ObjDriftice* this, s16* arg2, s16* temp_f22 *= arg0->unk_08; - //! FAKE: - *arg2 = arg0->unk_00[2] + (s32)((void)0, temp_f22); + *arg2 = (s32)temp_f22 + arg0->unk_04; } void func_80A66C4C(ObjDrifticeStruct4* arg0, ObjDriftice* this, s16* arg2, s16* arg3) { diff --git a/src/overlays/actors/ovl_Obj_Driftice/z_obj_driftice.h b/src/overlays/actors/ovl_Obj_Driftice/z_obj_driftice.h index 19627c3dfe..f77e95effd 100644 --- a/src/overlays/actors/ovl_Obj_Driftice/z_obj_driftice.h +++ b/src/overlays/actors/ovl_Obj_Driftice/z_obj_driftice.h @@ -23,8 +23,10 @@ typedef struct { } ObjDrifticeStruct3; // size = 0x14 typedef struct { - /* 0x00 */ s16 unk_00[2]; - /* 0x04 */ f32 unk_04; + /* 0x00 */ s16 unk_00; + /* 0x02 */ s16 unk_02; + /* 0x04 */ s16 unk_04; + /* 0x06 */ s16 unk_06; /* 0x08 */ f32 unk_08; /* 0x0C */ ObjDrifticeStruct3 unk_0C[2]; } ObjDrifticeStruct2; // size = 0x34 diff --git a/tools/disasm/variables.txt b/tools/disasm/variables.txt index 81cb2e4cd6..83d8d4db7a 100644 --- a/tools/disasm/variables.txt +++ b/tools/disasm/variables.txt @@ -1079,12 +1079,9 @@ 0x801BF9C4:("D_801BF9C4","UNK_TYPE1","",0x1), 0x801BF9C8:("D_801BF9C8","UNK_TYPE1","",0x1), 0x801BF9CC:("D_801BF9CC","UNK_TYPE1","",0x1), - 0x801BF9D4:("D_801BF9D4","UNK_TYPE4","",0x4), - 0x801BF9D8:("D_801BF9D8","UNK_TYPE4","",0x4), - 0x801BF9DC:("D_801BF9DC","UNK_TYPE4","",0x4), - 0x801BF9E0:("D_801BF9E0","UNK_TYPE4","",0x4), - 0x801BF9E4:("D_801BF9E4","UNK_TYPE4","",0x4), - 0x801BF9E8:("D_801BF9E8","UNK_TYPE4","",0x4), + 0x801BF9D4:("D_801BF9D4","UNK_TYPE4","",0x8), + 0x801BF9DC:("D_801BF9DC","UNK_TYPE4","",0x8), + 0x801BF9E4:("D_801BF9E4","UNK_TYPE4","",0x8), 0x801BF9EC:("sFinalHoursClockDigitsRed","UNK_TYPE4","",0x4), 0x801BF9F0:("sFinalHoursClockFrameEnvRed","UNK_TYPE2","",0x2), 0x801BF9F4:("sFinalHoursClockFrameEnvGreen","UNK_TYPE2","",0x2), @@ -1103,8 +1100,7 @@ 0x801BFAC4:("magicBorderIndices","UNK_TYPE1","",0x1), 0x801BFACC:("magicBorderColorTimerIndex","UNK_TYPE1","",0x1), 0x801BFAD4:("cUpLabelTextures","UNK_TYPE1","",0x1), - 0x801BFAF4:("D_801BFAF4","UNK_TYPE4","",0x4), - 0x801BFAF8:("D_801BFAF8","UNK_TYPE4","",0x4), + 0x801BFAF4:("D_801BFAF4","UNK_TYPE4","",0x8), 0x801BFAFC:("D_801BFAFC","UNK_TYPE4","",0x4), 0x801BFB04:("D_801BFB04","UNK_TYPE1","",0x1), 0x801BFB0C:("D_801BFB0C","UNK_TYPE1","",0x1), @@ -6856,8 +6852,7 @@ 0x808DE3E4:("D_808DE3E4","f32","",0x4), 0x808DE5B0:("D_808DE5B0","UNK_TYPE1","",0x1), 0x808DFEA0:("En_Horse_Link_Child_InitVars","UNK_TYPE1","",0x1), - 0x808DFEC0:("D_808DFEC0","UNK_TYPE4","",0x4), - 0x808DFEC8:("D_808DFEC8","UNK_TYPE4","",0x4), + 0x808DFEC0:("D_808DFEC0","UNK_TYPE4","",0x14), 0x808DFED4:("D_808DFED4","UNK_TYPE1","",0x1), 0x808DFEF8:("D_808DFEF8","UNK_TYPE1","",0x1), 0x808DFF08:("D_808DFF08","UNK_TYPE1","",0x1),