Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide AVOID_UB for some bugs found in GCC compiler testing #1785

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/z64player.h
Original file line number Diff line number Diff line change
Expand Up @@ -1421,7 +1421,7 @@ void Player_SetAutoLockOnActor(struct PlayState* play, Actor* actor);
s32 Player_SetBButtonAmmo(struct PlayState* play, s32 ammo);
bool Player_IsBurningStickInRange(struct PlayState* play, Vec3f* pos, f32 xzRange, f32 yRange);
u8 Player_GetStrength(void);
PlayerMask Player_GetMask(struct PlayState* play);
s32 Player_GetMask(struct PlayState* play);
void Player_RemoveMask(struct PlayState* play);
bool Player_HasMirrorShieldEquipped(struct PlayState* play);
bool Player_IsHoldingMirrorShield(struct PlayState* play);
Expand Down
2 changes: 1 addition & 1 deletion src/boot/yaz0.c
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ s32 Yaz0_DecompressImpl(u8* src, u8* dst) {
// N = chunkSize; B = back offset
// 3 bytes 0B BB NN
// 2 bytes NB BB
chunkSize = (nibble == 0) ? *src++ + 0x12 : nibble + 2;
chunkSize = (nibble == 0) ? (u32)(*src++ + 0x12) : nibble + 2;

do {
*dst++ = *(backPtr++ - 1);
Expand Down
19 changes: 19 additions & 0 deletions src/code/z_message_nes.c
Original file line number Diff line number Diff line change
Expand Up @@ -1005,8 +1005,23 @@ void Message_DecodeNES(PlayState* play) {
s16 value;
u32 timeToMoonCrash;
s16 i;
#ifndef AVOID_UB
// UB: digits is accessed out-of-bounds below (see bug annotation).
// On the IDO compiler the stack in memory is in the reverse
// order to variable declarations, so this ends up accessing
// numLines.
s16 numLines;
s16 digits[4];
#else
// Make this behavior consistent across compilers that allocate
// stack differently.
struct {
s16 digits[4];
s16 numLines;
} forceLayout;
#define numLines (forceLayout.numLines)
#define digits (forceLayout.digits)
#endif
s16 spC6 = 0;
u16 sfxHi;
f32 var_fs0;
Expand Down Expand Up @@ -1940,4 +1955,8 @@ void Message_DecodeNES(PlayState* play) {
decodedBufPos++;
msgCtx->msgBufPos++;
}
#ifdef AVOID_UB
#undef numLines
#undef digits
#endif
}
13 changes: 12 additions & 1 deletion src/code/z_player_lib.c
Original file line number Diff line number Diff line change
Expand Up @@ -1307,6 +1307,17 @@ struct_80124618 D_801C0560[] = {
{ 2, { 95, 95, 100 } },
{ 3, { 105, 105, 100 } },
{ 5, { 102, 102, 102 } },
#ifdef AVOID_UB
//! @bug gPlayerAnim_pz_gakkiplay uses this array with a frame count
//! of up to (and including) 6, which is larger than the last
//! keyframe frame number (5). This causes it to continue to read into
//! the next array in search of a keyframe that bounds frame 6.
// Avoid UB: Provide extra data elements that would be read in an
// out-of-bounds read from the next array. Both are read-only so are
// not expected to change.
{ 0, { 100, 100, 100 } },
{ 9, { 100, 100, 100 } },
#endif
};
struct_80124618 D_801C0580[] = {
{ 0, { 100, 100, 100 } }, { 9, { 100, 100, 100 } }, { 10, { 150, 150, 150 } },
Expand Down Expand Up @@ -1581,7 +1592,7 @@ u8 Player_GetStrength(void) {
return sPlayerStrengths[GET_PLAYER_FORM];
}

PlayerMask Player_GetMask(PlayState* play) {
s32 Player_GetMask(PlayState* play) {
Player* player = GET_PLAYER(play);

return player->currentMask;
Expand Down
15 changes: 12 additions & 3 deletions src/overlays/actors/ovl_En_Dnq/z_en_dnq.c
Original file line number Diff line number Diff line change
Expand Up @@ -456,9 +456,18 @@ void func_80A52FB8(EnDnq* this, PlayState* play) {

void EnDnq_HandleCutscene(EnDnq* this, PlayState* play) {
static s32 sCsAnimIndex[] = {
DEKU_KING_ANIM_IDLE, DEKU_KING_ANIM_IDLE_MORPH,
DEKU_KING_ANIM_SURPRISE, DEKU_KING_ANIM_JUMPED_ON_START,
DEKU_KING_ANIM_JUMPED_ON_END, DEKU_KING_ANIM_JUMPED_ON_END_MORPH,
DEKU_KING_ANIM_IDLE,
DEKU_KING_ANIM_IDLE_MORPH,
DEKU_KING_ANIM_SURPRISE,
DEKU_KING_ANIM_JUMPED_ON_START,
DEKU_KING_ANIM_JUMPED_ON_END,
DEKU_KING_ANIM_JUMPED_ON_END_MORPH,
#ifdef AVOID_UB
//! @bug Z2_DEKU_KINGCutsceneData_008258 provides a cue id of 6, reading out-of-bounds
//! of this array that happens to be 0 padding.
// Avoid UB: Add an explicit array member rather than relying on 0 padding.
DEKU_KING_ANIM_IDLE,
#endif
};
s32 cueChannel;
u32 cueId;
Expand Down
11 changes: 9 additions & 2 deletions src/overlays/actors/ovl_player_actor/z_player.c
Original file line number Diff line number Diff line change
Expand Up @@ -5671,7 +5671,7 @@ void func_80833864(PlayState* play, Player* this, PlayerMeleeWeaponAnimation mel
}

// Accumulate consecutive slashes to do the "third slash" types
if ((meleeWeaponAnim != this->meleeWeaponAnimation) || (this->unk_ADD >= 3)) {
if (((s32)meleeWeaponAnim != this->meleeWeaponAnimation) || (this->unk_ADD >= 3)) {
this->unk_ADD = 0;
}

Expand Down Expand Up @@ -11406,7 +11406,7 @@ void func_808425B4(Player* this) {
* - Tatl C-up icon for hints
*/
void Player_UpdateInterface(PlayState* play, Player* this) {
DoAction doActionB;
s32 doActionB;
s32 sp38;

if (this != GET_PLAYER(play)) {
Expand Down Expand Up @@ -18584,12 +18584,19 @@ void func_80855218(PlayState* play, Player* this, struct_8085D910** arg2) {
}
}

//! @bug This array may be indexed with PLAYER_FORM_HUMAN, causing an out-of-bounds access
u16 D_8085D908[] = {
WEEKEVENTREG_30_80, // PLAYER_FORM_FIERCE_DEITY
WEEKEVENTREG_30_20, // PLAYER_FORM_GORON
WEEKEVENTREG_30_40, // PLAYER_FORM_ZORA
WEEKEVENTREG_30_10, // PLAYER_FORM_DEKU
#ifdef AVOID_UB
// Avoid UB: Provide the data that would be read by indexing this with PLAYER_FORM_HUMAN.
// Both this array and D_8085D910 are read-only so this is not expected to change.
WEEKEVENTREG_16_02 | WEEKEVENTREG_16_08, // PLAYER_FORM_HUMAN
#endif
};

struct_8085D910 D_8085D910[] = {
{ 0x10, 0xA, 0x3B, 0x3F },
{ 9, 0x32, 0xA, 0xD },
Expand Down