Builds of RSDKv4 Crash with `-Wp,-D_FORTIFY_SOURCE=3` crash when loading Modded Content

Sewer56 commented 1 month ago

Before opening this issue, I ensure that...

[X] I have read the FAQ and confirmed my issue is not mentioned in it.
[X] I have checked both open and closed issues and confirmed this bug has not already been reported.
[X] I am not asking for tech support (e.g. game closing on startup, error when trying to build, etc).
[X] I am not asking for modding support or reporting a bug in a specific mod; this bug either occurs without mods enabled or is directly related to the mod loader.
[X] This bug is not related to any unofficial fork/port of the decompilation, and any issues with those should be kept in the appropriate repository.

Expected Behavior

Loading a stage with mod(s) enabled should not crash the process.

Actual Behavior

Loading a stage with mod(s) enabled crashes the process.

Steps to Reproduce

For convenience, I provide a sample PKGBUILD for Arch based distros below:

[On other Distros you can use CLI as usual]

pkgname=rsdkv4
pkgver=1.3.2
pkgrel=1
pkgdesc="Complete decompilation of Sonic 1 & Sonic 2 (2013) & Retro Engine (v4)"
arch=(x86_64)
url="https://github.com/Rubberduckycooly/Sonic-1-2-2013-Decompilation"
license=(custom)
depends=(glibc gcc-libs sdl2 glew libvorbis libglvnd)
makedepends=(git cmake)
source=("rsdkv4::git+https://github.com/Rubberduckycooly/Sonic-1-2-2013-Decompilation.git#tag=${pkgver}")
sha256sums=('SKIP')

prepare() {
  cd rsdkv4
  # Initialize and update submodules as per the original repository configuration
  git submodule update --init --recursive
}

build() {
  export CFLAGS="-march=native -O3 -pipe -fno-plt -fexceptions -Wp,-D_FORTIFY_SOURCE=3 -Wformat -Werror=format-security -fstack-clash-protection -fcf-protection"
  export CXXFLAGS="$CFLAGS -Wp,-D_GLIBCXX_ASSERTIONS"
  export LDFLAGS="-flto=auto"

  # We set RETRO_FORCE_CASE_INSENSITIVE in case of improper use of Mod API
  # by people making mods; an end user should have something that 'just works'
  # out of the box after all.
  cmake -B build -S "rsdkv4" -DRETRO_FORCE_CASE_INSENSITIVE=on
  cmake --build build --config release
}

package() {
  # Install the main binary
  install -D build/RSDKv4 -t "${pkgdir}/usr/bin"
  # Symlink to lowercase
  ln -s /usr/bin/RSDKv4 "${pkgdir}/usr/bin/rsdkv4"

  # Install the license file
  install -D rsdkv4/LICENSE.md -t "${pkgdir}/usr/share/licenses/${pkgname}"
}

This can be used with any Arch based distro with makepkg -si, or makepkg -siCf to force a clean rebuild.

In this PKGBUILD I specifically set the CFLAGS, CXXFLAGS and LDFLAGS explicitly for testing purposes. I set them to the default values you would encounter in /etc/makepkg.conf on any Arch based distro.

When building with -Wp,-D_FORTIFY_SOURCE=3 the Application crashes. This is likely indicative of a buffer overrun somewhere in RSDKv4 decomp.

I don't currently know where; my experience with C stuff is very little and I'm kind of in a rush to get SHC2024 entry evals done. :sweat_smile:

Running with AddressSanitizer/asan, there's an unrelated buffer overrun on boot, so I haven't gotten around to catching the actual issue (yet).

Reproduction Steps:

Enable a mod
Reboot game
Start a stage with modified assets

S314P can be used for testing, but I've experienced this in just about every mod.

Screenshots

No response

Log File

No response

Decompilation Version

1.3.2

Game

Sonic 2

Game Version

Mobile (Pre-Sega Forever)

Game Revision

No response

Platform

Linux x64 (Linux 6.11.0-5-cachyos-lto, x86-64-v3)

Additional Comments

No response

Mefiresu commented 1 month ago

All decomps are actually kinda broken when _FORTIFY_SOURCE is defined. Simplest workaround is obviously to unset it when building (which is actually what we do in github actions). Last time I checked it was because the library we use for ini parsing just straight up fails to read/write anything whenever _FORTIFY_SOURCE is defined. As for the ASan issues, you could try applying this PR.

Sewer56 commented 1 month ago

I did actually try applying #439 , something on boot however is tripping ASan nonetheless. Will report back later. Am extremely sleepy.

I haven't had the chance to check what it is yet (because I was testing with PKGBUILD and that was stripping symbols).

Mefiresu commented 1 month ago

No worries, I'll do some research on my end as well.

Mefiresu commented 1 month ago

On RSDKv4 the engine actually crashes during script parsing, for example in S314P:

DEBUG: StageSetup_HandleOscillationTable (strlen: 33)
*** buffer overflow detected ***: terminated
[1]    30637 IOT instruction (core dumped)  ./RSDKv4

On this line:

StrCopy(scriptFunctionList[scriptFunctionCount++].name, funcName);

The function name is to blame, it's apparently too long for the script compiler to handle (original code limitation):

struct ScriptFunction {
    byte access;
#if RETRO_USE_COMPILER
    char name[0x20];
#endif
    ScriptPtr ptr;
};

My guess as to why it doesn't completely break when _FORTIFY_SOURCE is unset is because functions are close to that limit and they're overflowing into the 3 bytes-wide struct padding between name and ptr.

Sewer56 commented 1 month ago

I just woke up, and oh, I can actually answer this (from my phone, even). You're on the money.

See the log:

DEBUG: StageSetup_HandleOscillationTable (strlen: 33)

It says strlen: 33.

On a 64-bit system I would expect the struct layout to be the following:

struct ScriptFunction {
    byte access;             // Offset: 0x00, Size: 1 byte
    char name[0x20];         // Offset: 0x01, Size: 32 bytes
    // Padding: 3 bytes to align the next member
    ScriptPtr ptr;           // Offset: 0x24, Size: 8 bytes (2x i32)
};

ScriptPtr are two i32(s), so the field alignment of the ScriptPtr struct in C rules is i32 (4). Structs usually align to largest member to ensure aligned reads/writes, but both members are the same here.

In this case because the string length is 33, it writes into the first byte of the padding between name and ptr.

Hopefully this should align with what you're thinking.

Mefiresu commented 1 month ago

This is exactly what happens yes, you can easily test this by editing the script file to extend StageSetup_HandleOscillationTable to 35 or more characters where it starts corrupting the ScriptPtr ptr data:

We've double checked on Sonic Origins and the behavior is identical, so this isn't technically a decomp specific bug, but rather an undocumented script limitation that some mods don't adhere to.

Sewer56 commented 1 month ago

What would be the following steps for a bug lile this then. Don't know the standard procedures for the repo.

Would you be extending the limit when the RETRO_ORIGINAL_CODE flag is not defined?

I imagine in any case, mod authors should be warned when they make names that are too long.

Most important thing I imagine is showing a MessageBox with the error details, but I'm not sure that would be the best approach given all the external forks that aren't X11/Wayland/Win32 etc. I would imagine an in-game message may be more annoying to show. But it may also be fine on the other hand to just MessageBox, not too many people writing scripts directly on homebrewed consoles.

MegAmi24 commented 1 month ago

Would you be extending the limit when the RETRO_ORIGINAL_CODE flag is not defined? I imagine in any case, mod authors should be warned when they make names that are too long.

We won't be extending the limit, since we want to keep parity with the official engine/Origins, but it may be worth adding a check for the function name length when that flag is disabled instead.

I would imagine an in-game message may be more annoying to show.

As seen above, there's already a system for in-game script error messages, so it wouldn't be that annoying.

Sewer56 commented 1 month ago

Ah, I see. I was more worried in an edge case where scripts may be loaded before the person making the mods could get into the Dev Menu itself; but I don't know the exact internals of how RSDK works. Specifically if a user were to enable a mod, close the game, and reopen it. If the script kicks and causes an overflow before the user can get into Dev Menu to see the error, that'd be problematic.

Mefiresu commented 1 month ago

What would be the following steps for a bug lile this then. Don't know the standard procedures for the repo. Would you be extending the limit when the RETRO_ORIGINAL_CODE flag is not defined?

Depends. In that particular case this bug is accurate to the original, but can cause issues if the name is >=35 chars long. Extending the limit using a RETRO_ORIGINAL_CODE ifdef may be a good idea as long as it doesn't break mods on Origins: extending it to 35 chars only will essentially keep the original behavior while also preventing buffer overflows caused by _FORTIFY_SOURCE being set.

I imagine in any case, mod authors should be warned when they make names that are too long.

The screenshot above is basically as close as we can get from a cross platform message box, and is actually what we implemented in the past here. We could definitely do something like this.

Mefiresu commented 1 month ago

Let's wait until SHC is over before breaking stuff :sweat_smile:

Sewer56 commented 1 month ago

I think there's also another caveat to mention here. Developers like us, no issue however...

An end user loading a mod should expect it to 'just work', regardless of when the mod was made. Consider the following mod categories:

Older version(s) of mods.
Mod where Author is MIA.
Mods from archives like the SHC Vault

The common element here, is mods within these categories may not be updated anymore. Because they cannot be updated anymore, the main RSDKv4 binary must instead properly ensure they are supported.

In other words, mods in those categories rely on the fact that the name buffer can be <=34 chars long rather than the intended <= 32 chars. Regardless of whether it is technically implementation detail due to compiler inserted padding; it worked in past official builds, so it's now considered part of the 'API contract' so to speak.

Mefiresu commented 1 month ago

This is what my fix proposal aimed to do, but we're very lucky that it can be done here.

We're sometimes forced to make breaking changes for the sake of accuracy (supporting Origins updates or just genuine inaccuracies like script syntax being different than the original) and general stability (undefined behaviors, non big-endian friendly code...).

It's also not feasible to maintain N versions of the decomp for all mods to work (that's what old decomp releases are for, fixed ), code bloat is already high enough as it is.

For fixes that can easily be backwards compatible like this one though? Goes without saying.

RSDKModding / RSDKv4-Decompilation