Note: For GCC there are still a huge number of `-Warray-bounds` warnings
coming from `externals/dynarmic`. I could have added a workaround in
`externals/CMakeLists.txt` similar to what this PR does for other
externals, but given Dynarmic's close affiliation with Yuzu, it would be
better to fix it upstream.
Besides that, on my machine, this makes the build warning-free except
for some warnings from glslangValidator and AutoMoc.
Details:
- Disable some warnings in externals.
- Disable `-Wnullability-completeness`, which is a Clang warning triggered
by the Vulkan SDK where if any pointers in the header are marked
_Nullable, it wants all pointers to be marked _Nullable or _Nonnull.
Most of them are, but some aren't. Who knows why.
- `src/web_service/verify_user_jwt.cpp`: Disable another warning when
including `jwt.hpp`.
- `src/input_common/input_poller.cpp`: Add missing `override` specifiers.
- src/common/swap.h: Remove redundant `operator&`. In general, this
file declares three overloads of each operator. Using `+` as an
example, the overloads are:
- a member function for `swapped_t + integer`
- a member function for `swapped_t + swapped_t`
- a free function for `integer + swapped_t`
But for `operator&`, there was an additional free function for
`swapped_t + integer`, which was redundant with the member function.
This caused a GCC warning saying "ISO C++ says that these are
ambiguous".
- On Unix, this would previously kill the Yuzu process with SIGPIPE.
Send MSG_NOSIGNAL to opt out of this.
- Add support for the proper error code in this situation, EPIPE.
- Windows has nonstandard behavior in this situation; translate it to
the standard behavior. Kind of pointless, but isn't it nice to be
correct?
There are still some other issues not addressed here, but it's a start.
Workarounds for false-positive reports:
- `RasterizerAccelerated`: Put a gigantic array behind a `unique_ptr`,
because UBSan has a [hardcoded limit](https://stackoverflow.com/questions/64531383/c-runtime-error-using-fsanitize-undefined-object-has-a-possibly-invalid-vp)
of how big it thinks objects can be, specifically when dealing with
offset-to-top values used with multiple inheritance. Hopefully this
doesn't have a performance impact.
- `QueryCacheBase::QueryCacheBase`: Avoid an operation that UBSan thinks
is UB even though it at least arguably isn't. See the link in the
comment for more information.
Fixes for correct reports:
- `PageTable`, `Memory`: Use `uintptr_t` values instead of pointers to
avoid UB from pointer overflow (when pointer arithmetic wraps around
the address space).
- `KScheduler::Reload`: `thread->GetOwnerProcess()` can be `nullptr`;
avoid calling methods on it in this case. (The existing code returns
a garbage reference to a field, which is then passed into
`LoadWatchpointArray`, and apparently it's never used, so it's
harmless in practice but still triggers UBSan.)
- `KAutoObject::Close`: This function calls `this->Destroy()`, which
overwrites the beginning of the object with junk (specifically a free
list pointer). Then it calls `this->UnregisterWithKernel()`. UBSan
complains about a type mismatch because the vtable has been
overwritten, and I believe this is indeed UB. `UnregisterWithKernel`
also loads `m_kernel` from the 'freed' object, which seems to be
technically safe (the overwriting doesn't extend as far as that
field), but seems dubious. Switch to a `static` method and load
`m_kernel` in advance.
- Add missing virtual destructor on `SSLBackend`.
- On Windows, filter out `POLLWRBAND` (one of the new flags added) when
calling `WSAPoll`, because despite the constant being defined on
Windows, passing it calls `WSAPoll` to yield `EINVAL`.
- Reduce OpenSSL version requirement to satisfy CI; I haven't tested
whether it actually builds (or runs) against 1.1.1, but if not, I'll
figure it out.
- Change an instance of memcpy to memmove, even though the arguments
cannot overlap, to avoid a [strange GCC
error](https://github.com/yuzu-emu/yuzu/pull/10912#issuecomment-1606283351).
The original name `larg` was copied from the OpenSSL documentation and
is not a typo of 'large' but rather an abbreviation of '`long`
argument'. But whatever, no harm in adding an underscore.
This implements some missing network APIs including a large chunk of the SSL
service, enough for Mario Maker (with an appropriate mod applied) to connect to
the fan server [Open Course World](https://opencourse.world/).
Connecting to first-party servers is out of scope of this PR and is a
minefield I'd rather not step into.
## TLS
TLS is implemented with multiple backends depending on the system's 'native'
TLS library. Currently there are two backends: Schannel for Windows, and
OpenSSL for Linux. (In reality Linux is a bit of a free-for-all where there's
no one 'native' library, but OpenSSL is the closest it gets.) On macOS the
'native' library is SecureTransport but that isn't implemented in this PR.
(Instead, all non-Windows OSes will use OpenSSL unless disabled with
`-DENABLE_OPENSSL=OFF`.)
Why have multiple backends instead of just using a single library, especially
given that Yuzu already embeds mbedtls for cryptographic algorithms? Well, I
tried implementing this on mbedtls first, but the problem is TLS policies -
mainly trusted certificate policies, and to a lesser extent trusted algorithms,
SSL versions, etc.
...In practice, the chance that someone is going to conduct a man-in-the-middle
attack on a third-party game server is pretty low, but I'm a security nerd so I
like to do the right security things.
My base assumption is that we want to use the host system's TLS policies. An
alternative would be to more closely emulate the Switch's TLS implementation
(which is based on NSS). But for one thing, I don't feel like reverse
engineering it. And I'd argue that for third-party servers such as Open Course
World, it's theoretically preferable to use the system's policies rather than
the Switch's, for two reasons
1. Someday the Switch will stop being updated, and the trusted cert list,
algorithms, etc. will start to go stale, but users will still want to
connect to third-party servers, and there's no reason they shouldn't have
up-to-date security when doing so. At that point, homebrew users on actual
hardware may patch the TLS implementation, but for emulators it's simpler to
just use the host's stack.
2. Also, it's good to respect any custom certificate policies the user may have
added systemwide. For example, they may have added custom trusted CAs in
order to use TLS debugging tools or pass through corporate MitM middleboxes.
Or they may have removed some CAs that are normally trusted out of paranoia.
Note that this policy wouldn't work as-is for connecting to first-party
servers, because some of them serve certificates based on Nintendo's own CA
rather than a publicly trusted one. However, this could probably be solved
easily by using appropriate APIs to adding Nintendo's CA as an alternate
trusted cert for Yuzu's connections. That is not implemented in this PR
because, again, first-party servers are out of scope.
(If anything I'd rather have an option to _block_ connections to Nintendo
servers, but that's not implemented here.)
To use the host's TLS policies, there are three theoretical options:
a) Import the host's trusted certificate list into a cross-platform TLS
library (presumably mbedtls).
b) Use the native TLS library to verify certificates but use a cross-platform
TLS library for everything else.
c) Use the native TLS library for everything.
Two problems with option a). First, importing the trusted certificate list at
minimum requires a bunch of platform-specific code, which mbedtls does not have
built in. Interestingly, OpenSSL recently gained the ability to import the
Windows certificate trust store... but that leads to the second problem, which
is that a list of trusted certificates is [not expressive
enough](https://bugs.archlinux.org/task/41909) to express a modern certificate
trust policy. For example, Windows has the concept of [explicitly distrusted
certificates](https://learn.microsoft.com/en-us/previous-versions/windows/it-pro/windows-server-2012-r2-and-2012/dn265983(v=ws.11)),
and macOS requires Certificate Transparency validation for some certificates
with complex rules for when it's required.
Option b) (using native library just to verify certs) is probably feasible, but
it would miss aspects of TLS policy other than trusted certs (like allowed
algorithms), and in any case it might well require writing more code, not less,
compared to using the native library for everything.
So I ended up at option c), using the native library for everything.
What I'd *really* prefer would be to use a third-party library that does option
c) for me. Rust has a good library for this,
[native-tls](https://docs.rs/native-tls/latest/native_tls/). I did search, but
I couldn't find a good option in the C or C++ ecosystem, at least not any that
wasn't part of some much larger framework. I was surprised - isn't this a
pretty common use case? Well, many applications only need TLS for HTTPS, and they can
use libcurl, which has a TLS abstraction layer internally but doesn't expose
it. Other applications only support a single TLS library, or use one of the
aforementioned larger frameworks, or are platform-specific to begin with, or of
course are written in a non-C/C++ language, most of which have some canonical
choice for TLS. But there are also many applications that have a set of TLS
backends just like this; it's just that nobody has gone ahead and abstracted
the pattern into a library, at least not a widespread one.
Amusingly, there is one TLS abstraction layer that Yuzu already bundles: the
one in ffmpeg. But it is missing some features that would be needed to use it
here (like reusing an existing socket rather than managing the socket itself).
Though, that does mean that the wiki's build instructions for Linux (and macOS
for some reason?) already recommend installing OpenSSL, so no need to update
those.
## Other APIs implemented
- Sockets:
- GetSockOpt(`SO_ERROR`)
- SetSockOpt(`SO_NOSIGPIPE`) (stub, I have no idea what this does on Switch)
- `DuplicateSocket` (because the SSL sysmodule calls it internally)
- More `PollEvents` values
- NSD:
- `Resolve` and `ResolveEx` (stub, good enough for Open Course World and
probably most third-party servers, but not first-party)
- SFDNSRES:
- `GetHostByNameRequest` and `GetHostByNameRequestWithOptions`
- `ResolverSetOptionRequest` (stub)
## Fixes
- Parts of the socket code were previously allocating a `sockaddr` object on
the stack when calling functions that take a `sockaddr*` (e.g. `accept`).
This might seem like the right thing to do to avoid illegal aliasing, but in
fact `sockaddr` is not guaranteed to be large enough to hold any particular
type of address, only the header. This worked in practice because in
practice `sockaddr` is the same size as `sockaddr_in`, but it's not how the
API is meant to be used. I changed this to allocate an `sockaddr_in` on the
stack and `reinterpret_cast` it. I could try to do something cleverer with
`aligned_storage`, but casting is the idiomatic way to use these particular
APIs, so it's really the system's responsibility to avoid any aliasing
issues.
- I rewrote most of the `GetAddrInfoRequest[WithOptions]` implementation. The
old implementation invoked the host's getaddrinfo directly from sfdnsres.cpp,
and directly passed through the host's socket type, protocol, etc. values
rather than looking up the corresponding constants on the Switch. To be
fair, these constants don't tend to actually vary across systems, but
still... I added a wrapper for `getaddrinfo` in
`internal_network/network.cpp` similar to the ones for other socket APIs, and
changed the `GetAddrInfoRequest` implementation to use it. While I was at
it, I rewrote the serialization to use the same approach I used to implement
`GetHostByNameRequest`, because it reduces the number of size calculations.
While doing so I removed `AF_INET6` support because the Switch doesn't
support IPv6; it might be nice to support IPv6 anyway, but that would have to
apply to all of the socket APIs.
I also corrected the IPC wrappers for `GetAddrInfoRequest` and
`GetAddrInfoRequestWithOptions` based on reverse engineering and hardware
testing. Every call to `GetAddrInfoRequestWithOptions` returns *four*
different error codes (IPC status, getaddrinfo error code, netdb error code,
and errno), and `GetAddrInfoRequest` returns three of those but in a
different order, and it doesn't really matter but the existing implementation
was a bit off, as I discovered while testing `GetHostByNameRequest`.
- The new serialization code is based on two simple helper functions:
```cpp
template <typename T> static void Append(std::vector<u8>& vec, T t);
void AppendNulTerminated(std::vector<u8>& vec, std::string_view str);
```
I was thinking there must be existing functions somewhere that assist with
serialization/deserialization of binary data, but all I could find was the
helper methods in `IOFile` and `HLERequestContext`, not anything that could
be used with a generic byte buffer. If I'm not missing something, then
maybe I should move the above functions to a new header in `common`...
right now they're just sitting in `sfdnsres.cpp` where they're used.
- Not a fix, but `SocketBase::Recv`/`Send` is changed to use `std::span<u8>`
rather than `std::vector<u8>&` to avoid needing to copy the data to/from a
vector when those methods are called from the TLS implementation.
`network.cpp` has several error paths which either:
- report "Unhandled host socket error=n" and return `SUCCESS`, or
- switch on a few possible errors, log them, and translate them to
Errno; the same switch statement is copied and pasted in multiple
places in the code
Convert these paths to use a helper function `GetAndLogLastError`, which
is roughly the equivalent of one of the switch statements, but:
- handling more cases (both ones that were already in `Errno`, and a few
more I added), and
- using OS functions to convert the error to a string when logging, so
it'll describe the error even if it's not one of the ones in the
switch statement.
- To handle this, refactor the logic in `GetLastErrorMsg` to expose a
new function `NativeErrorToString` which takes the error number
explicitly as an argument. And improve the Windows version a bit.
Also, add a test which exercises two random error paths.
Found this via a warning, but it's a substantive fix.
Since this is only for a cache, it should be safe to silently drop the
entry if opening fails. I think.
- For `std::same_as`, add missing include of `<concepts>`.
- For `std::convertible_to`, create a replacement in `common/concepts.h`
and use that instead.
This would also be found in `<concepts>`, but unlike `std::same_as`,
`std::convertible_to` is not yet implemented in libc++, LLVM's STL
implementation - not even in master. (In fact, `std::same_as` is the
*only* concept currently implemented. For some reason.)
Since this is inside a string literal, backslashes that are part of
regex syntax have to be escaped. But that's ugly, so convert to a raw
string instead.
Actually, two enum classes, since for some reason there are two separate
yet identical `PollFD` types used in the codebase. I get that one is
ABI-compatible with the Switch while the other is an abstract type used
for the host, but why not use `WSAPOLLFD` directly for the latter?
Anyway, why make this change? Because on Apple platforms, `POLL_IN`,
`POLL_OUT`, etc. (with an underscore) are defined as macros in
<sys/signal.h>. (This is inherited from FreeBSD.) So defining
a variable with the same name causes a compile error.
I could just rename the variables, but while I was at it I thought I
might as well switch to an enum for stronger typing.
Also, change the type used for values copied directly to/from the
`events` and `revents` fields of the host *native*
`pollfd`/`WSASPOLLFD`, from `u32` to `short`, as `short` is the correct
canonical type on both Unix and Windows.
`PhysicalCore`'s move assignment operator was declared as `= default`,
but was implicitly deleted because `PhysicalCore` has fields
of reference type. Switch to explicitly deleting it to avoid a Clang
warning.
The move *constructor* is still defaulted, and is required to exist due
to the use of `std::vector<PhysicalCore>`.
- Add a type check so that calling Push with an invalid type produces a
compile error rather than a linker error.
- vi.cpp was calling Push with a variable of type `std::size_t`.
There's no explicit overload for `size_t`, but there is one for `u64`,
which on most platforms is the same type as `size_t`. On macOS,
however, it isn't: both types are 64 bits, but `size_t` is `unsigned
long` and `u64` is `unsigned long long`. Regardless, it makes more
sense to explicitly use `u64` here instead of `size_t`.
The previous definition was:
#define NUM(field_name) (sizeof(Maxwell3D::Regs::field_name) / sizeof(u32))
In cases where `field_name` happens to refer to an array, Clang thinks
`sizeof(an array value) / sizeof(a type)` is an instance of the idiom
where `sizeof` is used to compute an array length. So it thinks the
type in the denominator ought to be the array element type, and warns if
it isn't, assuming this is a mistake.
In reality, `NUM` is not used to get array lengths at all, so there is no
mistake. Silence the warning by applying Clang's suggested workaround
of parenthesizing the denominator.
On Apple platforms, FALSE and TRUE are defined as macros by
<mach/boolean.h>, which is included by various system headers.
Note that there appear to be no actual users of the names to fix up.
Specifically:
const auto size = sdl2_config->GetInteger("System", "users_size", 0);
The variable is never used, producing a warning. I wondered if this
ought to be assigning something to in `Settings`, but nothing else in
the codebase ever mentions a setting called "users_size", so I guess
it's safe to remove...
This Clang warning complains when offsetof is used on a
non-standard-layout type (i.e. any class using various C++ features),
even though it works fine (and is not undefined behavior as of C++17).
- Use .at() instead of raw indexing when dealing with untrusted indices.
- For the special case of WaitFence with syncpoint id UINT32_MAX,
instead of crashing, log an error and ignore. This is what I get when
running Super Mario Maker 2.
EmuWindow::PollEvents was called from the GPU thread (or the CPU thread
in sync-GPU mode) when swapping buffers. It had three implementations:
- In GRenderWindow, it didn't actually poll events, just set a flag and
emit a signal to indicate that a frame was displayed.
- In EmuWindow_SDL2_Hide, it did nothing.
- In EmuWindow_SDL2, it did call SDL_PollEvents, but this is wrong
because SDL_PollEvents is supposed to be called on the thread that set
up video - in this case, the main thread, which was sleeping in a
busyloop (regardless of whether sync-GPU was enabled). On macOS this
causes a crash.
To fix this:
- Rename EmuWindow::PollEvents to OnFrameDisplayed, and give it a
default implementation that does nothing.
- In EmuWindow_SDL2, do not override OnFrameDisplayed, but instead have
the main thread call SDL_WaitEvent in a loop.
- In `SetCurrentThreadName`, when on Linux, truncate to 15 bytes, as (at
least on glibc) `pthread_set_name_np` will otherwise return `ERANGE` and
do nothing.
- Also, add logging in case `pthread_set_name_np` returns an error
anyway. This is Linux-specific, as the Apple and BSD versions of
`pthread_set_name_np return `void`.
- Change the name for CPU threads in multi-core mode from
"yuzu:CoreCPUThread_N" (19 bytes) to "yuzu:CPUCore_N" (14 bytes) so it
fits into the Linux limit. Some other thread names are also cut off,
but I didn't bother addressing them as you can guess them from the
truncated versions. For a CPU thread, truncation means you can't see
which core it is!
