search

hrydgard / ppsspp

A PSP emulator for Android, Windows, Mac and Linux, written in C++. Want to contribute? Join us on Discord at https://discord.gg/5NJB6dD or just send pull requests / issues. For discussion use the forums at forums.ppsspp.org.
https://www.ppsspp.org
Other
11.07k stars 2.16k forks source link

Incorrect/bogus assets folder detection (ExePath related) #14483

Closed cmitu closed 3 years ago

cmitu commented 3 years ago

What happens?

PPSSPPSDL seems to incorrectly detect the ExePath when having a controller (DualShock 4) plugged in.

As a result, the assets folder is not correctly found and the UI doesn't correctly show. The logs show that the assets (SDL's gamescontrollerdb file, ui_atlas.zim, other files) cannot be loaded:

loading control pad mappings from gamecontrollerdb.txt: gamecontrollerdb.txt missing
SUCCESS!
....
56:10:202 UI/TextureUtil.cpp:155 E[IO]: Failed to read file 'ui_atlas.zim'
56:10:202 UI/TextureUtil.cpp:155 E[IO]: Failed to read file 'ui_atlas.zim'
56:10:202 UI/TextureUtil.cpp:155 E[IO]: Failed to read file 'ui_atlas.zim'

When starting PPSSPP without any controller connected, the UI is shown correctly and there are not assets related errors in the log. The installation folder contains the PPSSPPSDL executable and the assets folder.

I've added a log statement in GetExePath and this gets printed in the logs:

NOTE: As a workaround, starting PPSSPPSDL from the installation folder works, since the CWD's assets folder is added to the search list. Copying the assets folder to any of the other known locations (/usr/local/share/ppsspp) also works.

What should happen?

No errors related to any assets related file loading, UI should show up correctly.

What are you using?

What hardware / device and operating system?

Raspberry Pi 4 using Raspbery Pi OS (former Raspbian) Buster (armhf, no 64bit)

What graphics card (GPU) or mobile phone model?

Using the Pi4's VideoCore VI (GLES3 renderer)

What PPSSPP version (standalone/official), and did it work before?

Not sure, but might try a bisect later. Could be related to the recent changes in GetExeDirectory.

Which game or games?

No game loaded, just trying to start the emulator. Same errors are logged when loading a game (for instance Castlevania - The Dracula X in iso format) and the UI doesn't work.

Checklist

Compiled the latest master, --version shows 7a28919ca

unknownbrackets commented 3 years ago

Interesting, thanks for the investigation.

It should be using this path: https://github.com/hrydgard/ppsspp/blob/7a28919ca7fa3f4c1e81ad06c60ebceec9b1e2f8/Common/File/FileUtil.cpp#L758-L759

I wonder if it's an issue with it being returned as a reference. What if you change this:

const Path &GetExeDirectory() {
    static Path ExePath;

To:

const Path GetExeDirectory() {
    Path ExePath;

And then the matching change in the header? In theory, it's static, so this shouldn't matter.... but I'd be surprised if /proc/self/exe points to something with input/ in it. If it does, though, it'd seem like that's from a modification to argv[0].

https://github.com/hrydgard/ppsspp/blob/7a28919ca7fa3f4c1e81ad06c60ebceec9b1e2f8/SDL/SDLMain.cpp#L526 https://github.com/hrydgard/ppsspp/blob/7a28919ca7fa3f4c1e81ad06c60ebceec9b1e2f8/SDL/SDLMain.cpp#L670 https://github.com/hrydgard/ppsspp/blob/29ae351a523567685d259b34b4595fa96e19e02c/UI/NativeApp.cpp#L572

I don't think argv[0] should get modified, though.

Definitely interested in the results of any bisect.

-[Unknown]

cmitu commented 3 years ago

@unknownbrackets thank you for the reply.

It took a bit longer than I anticipated, but I managed to complete a bisect. git bisect reports:

2558022afe29fc9b3197aac5521e8dca728e0772 is the first bad commit
commit 2558022afe29fc9b3197aac5521e8dca728e0772
Author: @unknownbrackets.org
Date:   Sat May 15 09:32:41 2021 -0700

    Config: Move data path settings to Paths.
...

It's a bit puzzling, since the commit itself doesn't touch any of the codepaths that - I presume - would produce a faulty 'ExePath'.

I tried removing the static qualifier from ExePath (inGetExePath`) just crashes the emulator.

Since it's a very peculiar behavior, I'm going to try a debug build and maybe compare with a x86/x86_64 build. Maybe debugging and looking at a call stack would shed more light on this particular issue.

hrydgard commented 3 years ago

does feel like a memory corruption issue... maybe build with b.sh --debug --sanitize and see if you get any interesting output?

cmitu commented 3 years ago

Yes, it really does look like memory corruption. Building a debug version + adding ASAN doesn't exhibit the bug though.

I did however built a binary with CMAKE_BUILD_TYPE=RelWithDebInfo and used a breakpoint to see what is going on inside GetExeDirectory and it looks like program_name (declared here) gets set to some bogus memory address. Printing its contents shows something like:

...
DEBUG: Vulkan is not available, not using Vulkan.
Info: We compiled against SDL version 2.0.10 and we are linking against SDL version 2.0.10. :)

# `program_path` printed here, before `readlink` is invoked
3:1.3/0003:054C:09CC.000D/input/input32/capabilities/key at program_path

27:13:028 Core/Config.cpp:544 I[G3D]: Longest display side: -1 pixels. Choosing scale 1
...

Since readlink doesn't add the \0, the bogus path that results makes sense given the exe path (/home/pi/ppsspp), resulting in something like the path that I reported in the first post (/home/pi/ppsspp/PPSSPPSDL/input/input21/capabilities).

Sure enough, zero-ing program_path's contents (via memset) before calling readlink here solves the issue.

Maybe a compiler mis-optimization (since the pure debug + ASAN build doesn't trigger the issue) ?

EDIT: spelling.

unknownbrackets commented 3 years ago

Oh, oops. So it was missing a zero initialization. Added at least that in ecc2f62.

I guess char *last_slash = strrchr(program_path, '/'); randomly made this work, and that's why this wasn't noticed before? Maybe something was lucky enough to usually leave some zeros on the stack before.

Thanks for digging into this and your investigation.

-[Unknown]

cmitu commented 3 years ago

Oh, oops. So it was missing a zero initialization. Added at least that in ecc2f62.

That fixes it, thank you.

hrydgard commented 3 years ago

Nicely debugged!