HLE_OS: Manually handle printfs from emulated software to prevent emulated software from crashing Dolphin with an invalid printf formatting string. #11025

AdmiralCurtiss · 2022-08-31T07:08:50Z

This is incomplete and not tested, I just was reminded that I had this. I may or may not come back to this, someone else is free to take this over if they want...

For reference, I'm unsure of the exact circumstances, but I have seen Dolphin crash from a game invoking a HLE-hooked printf(), which is what triggered me going down this path.

sepalani · 2022-08-31T14:29:40Z

Just wondering, can't fmt::sprintf be used and the format string parsing would only be done to grab the required arguments and store them into a fmt::dynamic_format_arg_store? I might give it a shot when I have the time.

AdmiralCurtiss · 2022-08-31T15:12:13Z

I think that still results in platform inconsistency (consider eg. platforms where long is 64 bit vs platforms where long is 32 bit), but feel free to give it a shot!

e: Also you have to filter out anything the hooked function wouldn't support, like positional arguments...

dreamsyntax · 2023-04-04T01:44:56Z

Shadow the Hedgehog - [GUPE8P, GUPP8P, GUPJ8P]
Would have a large reduction in Dolphin crashing to desktop (CTD) from this PR.
I have personally confirmed this PR affecting this game with a reproducible state I made where after ~10 seconds in a position a print triggers.
Because Dolphin currently defaults to Console Type : Development HW3 (10000006), this game's print functions run.

In this game OSReport EXI shows human readable strings, and OSReport HLE shows tons of junk every frame.
Occasionally, depending on the print Dolphin can CTD, below was one example.

800ae6c0->800ae6ac| �R�怙ｴV閨8�_'ﾘ

I confirmed after testing this build the crash to desktop is resolved, though there is a brief hiccup/stutter when it reaches the problematic log. The game continues emulation successfully.

Source/Core/Core/HLE/HLE_OS.cpp

shuffle2 · 2023-04-06T16:04:22Z

Source/Core/Core/PowerPC/MMU.cpp

+  do
+  {
+    if (!HostIsRAMAddress(guard, address) || !HostIsRAMAddress(guard, address + 1))
+      break;


should this return error instead of "partial" string?

Possibly... but if yes so should HostGetString().

AdmiralCurtiss · 2023-07-17T20:35:02Z

This has been sitting here long enough. I think it works, even if it's a bit ugly in spots, and it's definitely better than letting emulated software crash or worse write to arbitrary memory with a rogue printf statement.

sepalani

I'll review the rest of the PR, asap. I'm going to test it with a bunch of games I have which are displaying debug messages.

sepalani · 2023-07-22T06:18:16Z

Source/Core/Core/PowerPC/MMU.cpp

+    u16 res = HostRead_U16(guard, address);
+    if (!res)
+      break;
+    s += static_cast<u16>(res);


Can't res be const'd and is the static_cast necessary?

Oops, that's supposed to cast to char16_t.

sepalani · 2023-07-22T06:26:11Z

Source/Core/Core/HLE/HLE_OS.cpp

+  HLEPrintArgsVAList(const Core::CPUThreadGuard& guard, HLE::SystemVABI::VAList& va_list)
+      : m_guard(guard), m_va_list(va_list)
+  {


Shouldn't HLE::SystemVABI::VAList& va_list be passed as a pointer in case it's modified according to Dolphin's coding style? If the coding style has changed or allows some exceptions, they might be worth mentioning in the Contributing.md file.

Mhm, I think that's actually ambiguous, since we're not actually assigning a new value to the passed va_list, but yeah sure it doesn't hurt.

sepalani

Some more remarks.

I'm wondering, can't the remaining StringFromFormat be replaced with fmt::sprintf?

Source/Core/Core/HLE/HLE_OS.cpp

sepalani · 2023-07-29T08:45:57Z

Source/Core/Core/HLE/HLE_OS.cpp

      if (i >= string.size())
+      {
+        // not a valid formatting string, print the formatting string as-is
+        result += string.substr(formatting_start_position);
        break;
-
-      ArgumentBuffer += string[i];
+      }


Aren't you returning a partially formatted string instead of the original format string string? You're also skipping all the subsequent conversion specifications. I'm unsure of the behaviour on actual hardware/game/homebrew. I'll write some tests to see how they behave.

Well this is undefined behavior, it's reading past the end of the string. This is basically if you had something like eg. printf("text %s %0", "something", 5);; that last %0 has a padding_zeroes_flag but then nothing else after it. I'd argue printing text something %0 in this case is more useful than printing text %s %0 but ymmv, we can technically do whatever.

Source/Core/Core/HLE/HLE_OS.cpp

…ted software.

AdmiralCurtiss · 2023-07-29T14:13:32Z

I've also changed the StringFromFormat to fmt::sprintf. Sadly there's no fmt::sprintf_to but whatever, not like this is particularly performance-critical...

sepalani

Otherwise, this PR seems to work fine with the game I tested.

sepalani · 2023-07-29T17:59:44Z

Source/Core/Core/HLE/HLE_OS.cpp

+      {
+        fmt::format_to(std::back_inserter(result),
+                       fmt::runtime(left_justified_flag ? "{0:<{1}}" : "{0:>{1}}"),
+                       static_cast<unsigned char>(value), field_width.value_or(0));


After some testing, I had a regression. Casting the value as signed or unsigned char will display it as an integer. This should be casted as plain char instead.

Ah so just static_cast<char>(value)? Weird that that makes a difference.

Yeah, I was surprised too, apparently it was done to fix uint8_t and int8_t being displayed as character instead of integer.

…lated software from crashing Dolphin with an invalid printf formatting string.

AdmiralCurtiss · 2023-08-18T23:50:03Z

@sepalani Any further comments?

sepalani

LGTM. This PR is also better than the current way we're logging things. @dreamsyntax might want to double check that it still fixes the crashes on Shadow the Hedgehog.

Sadly, I'm unable to reproduce the crash mentioned by @dreamsyntax on Shadow the Hedgehog (PAL), with either this PR or master. Both tried to print nonsense still. There is no known symbol maps for this game, AFAIK, so Dolphin autodetection creates false positives.

I noticed one nlPrintf function at 8001c574 which doesn't seem log related but might use varargs. I was unable to figure out if its parameters point to valid strings.

Whereas, there is another function detected as ___blank at 8000d374 which might or might not be log related. However, it might be related to the boost library (v1.32, since the string occurrences don't exist prior to this version) and shared_ptr serialization. The first argument (r3) passed to this function when dereferenced several times points to r6 then a boost string. The r6 and r3 values are set right before the ___blank function call so there is a very high chance for them to be related.

Some examples (no surprise that dolphin autodection failed to find the string):

r3=8060c534 -> 807f07a0 -> r6=80560298 -> 805ee9a0 (char**) -> 80512824 (char*)
- string at 80512824: boost::detail::sp_counted_base_impl<Vibration::ManagerUnit *, boost::checked_deleter<Vibration::ManagerUnit>>
r3=8060c4f4 -> 807f0818 -> r6=8054ddac -> 805eba3c (char**) -> 804f7a10 (char*)
- string at 804f7a10: boost::detail::sp_counted_base_impl<SonicteamUSA::System::Camera::CameraScreen *, boost::checked_deleter<SonicteamUSA::System::Camera::CameraScreen>>

AdmiralCurtiss force-pushed the hle-printf branch from 95c63f2 to 3fdbf30 Compare January 24, 2023 07:22

AdmiralCurtiss force-pushed the hle-printf branch from 3fdbf30 to 252cb99 Compare March 5, 2023 18:43

AdmiralCurtiss force-pushed the hle-printf branch 2 times, most recently from b66be44 to 02acd40 Compare April 4, 2023 01:28

AdmiralCurtiss force-pushed the hle-printf branch 2 times, most recently from 63fcdbc to c955668 Compare April 5, 2023 17:53

AdmiralCurtiss mentioned this pull request Apr 5, 2023

Jit64: Fix possibly redundant MOV in ABI_CallFunctionPR(). #11729

Merged

AdmiralCurtiss force-pushed the hle-printf branch from c955668 to 202323c Compare April 5, 2023 18:32

dreamsyntax mentioned this pull request Apr 6, 2023

[WIP] Configurable Console Type per Game #11733

Draft

shuffle2 reviewed Apr 6, 2023

View reviewed changes

Source/Core/Core/HLE/HLE_OS.cpp Show resolved Hide resolved

shuffle2 reviewed Apr 6, 2023

View reviewed changes

AdmiralCurtiss force-pushed the hle-printf branch from 202323c to aa8c7d2 Compare April 10, 2023 18:02

AdmiralCurtiss force-pushed the hle-printf branch from aa8c7d2 to 0b174f2 Compare July 2, 2023 16:55

AdmiralCurtiss force-pushed the hle-printf branch from 0b174f2 to 04b8d51 Compare July 17, 2023 20:33

AdmiralCurtiss marked this pull request as ready for review July 17, 2023 20:33

sepalani suggested changes Jul 22, 2023

View reviewed changes

AdmiralCurtiss force-pushed the hle-printf branch 2 times, most recently from 326f81a to 86f9846 Compare July 22, 2023 13:14

sepalani suggested changes Jul 29, 2023

View reviewed changes

MMU: Add a HostGetU16String() function for wide strings used by emula…

7f29f03

…ted software.

AdmiralCurtiss force-pushed the hle-printf branch 3 times, most recently from 9e66d5f to 5bfc360 Compare July 29, 2023 14:12

sepalani suggested changes Jul 29, 2023

View reviewed changes

HLE_OS: Manually handle printfs from emulated software to prevent emu…

9f81791

…lated software from crashing Dolphin with an invalid printf formatting string.

AdmiralCurtiss force-pushed the hle-printf branch from 5bfc360 to 9f81791 Compare July 29, 2023 18:06

sepalani approved these changes Aug 19, 2023

View reviewed changes

AdmiralCurtiss merged commit f19651e into dolphin-emu:master Aug 19, 2023
11 checks passed

AdmiralCurtiss deleted the hle-printf branch August 19, 2023 23:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HLE_OS: Manually handle printfs from emulated software to prevent emulated software from crashing Dolphin with an invalid printf formatting string. #11025

HLE_OS: Manually handle printfs from emulated software to prevent emulated software from crashing Dolphin with an invalid printf formatting string. #11025

AdmiralCurtiss commented Aug 31, 2022 •

edited

sepalani commented Aug 31, 2022

AdmiralCurtiss commented Aug 31, 2022 •

edited

dreamsyntax commented Apr 4, 2023 •

edited

shuffle2 Apr 6, 2023

AdmiralCurtiss Jul 17, 2023

AdmiralCurtiss commented Jul 17, 2023

sepalani left a comment

sepalani Jul 22, 2023

AdmiralCurtiss Jul 22, 2023

sepalani Jul 22, 2023

AdmiralCurtiss Jul 22, 2023

sepalani left a comment

sepalani Jul 29, 2023

AdmiralCurtiss Jul 29, 2023

AdmiralCurtiss commented Jul 29, 2023

sepalani left a comment

sepalani Jul 29, 2023

AdmiralCurtiss Jul 29, 2023

sepalani Jul 29, 2023

AdmiralCurtiss commented Aug 18, 2023

sepalani left a comment

HLE_OS: Manually handle printfs from emulated software to prevent emulated software from crashing Dolphin with an invalid printf formatting string. #11025

HLE_OS: Manually handle printfs from emulated software to prevent emulated software from crashing Dolphin with an invalid printf formatting string. #11025

Conversation

AdmiralCurtiss commented Aug 31, 2022 • edited

sepalani commented Aug 31, 2022

AdmiralCurtiss commented Aug 31, 2022 • edited

dreamsyntax commented Apr 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdmiralCurtiss commented Jul 17, 2023

sepalani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sepalani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdmiralCurtiss commented Jul 29, 2023

sepalani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdmiralCurtiss commented Aug 18, 2023

sepalani left a comment

Choose a reason for hiding this comment

AdmiralCurtiss commented Aug 31, 2022 •

edited

AdmiralCurtiss commented Aug 31, 2022 •

edited

dreamsyntax commented Apr 4, 2023 •

edited