Prefer to grab the server's SDL descriptors. #1450
Conversation
This removes the singlular `/SkipPreload` command line argument in favor of two arguments: `/LocalPython` and `/LocalSDL`. `/LocalData` now implies the former but not the latter. The reason for this change is that the SDL is a contract between the server and the client about how exactly an SDL blob is formatted. If SDL blobs don't have the same format between the client and server, this can have fairly bad implications, ranging from remote players being disconnected with an error, the player with the wrong SDL getting an error, or the server crashing.
There was a problem hiding this comment.
This seems sensible to me - it will make it much easier to safely use a H'uru client on Cyan's shard. Of course, this can lead to the client using SDL that's incompatible with its local scripts and age data, but that's much less likely to cause remote breakage than SDL mismatches.
To clarify, because I don't fully understand all of the patching logic... does this apply only if the server provides the .sdl files via the auth server/secure preloader, or also if they're provided via the file server? Does this change anything about which of the two sources is preferred?
| hsLockGuard(fWorker->fRequestMut); | ||
| fWorker->fRequests.emplace_back("SecurePreloader", pfPatcherWorker::Request::kSecurePreloader); | ||
| if (NetCliFileQueryConnected()) { |
There was a problem hiding this comment.
All the other code checks gDataServerLocal directly - or is this condition supposed to check something different?
There was a problem hiding this comment.
gDataServer is a variable from plResManager, which I really didn't want to depend on in this case. Instead, what we're really interested in is whether or not a file server is available. If it isn't. then we should fall back to the auth server.
There was a problem hiding this comment.
what we're really interested in is whether or not a file server is available. If it isn't. then we should fall back to the auth server.
Ah, I see - that logic makes sense, and in that case I agree the new function is the better choice.
Co-authored-by: dgelessus <dgelessus@users.noreply.github.com>
Yeah, there's really not a good way to fix all of the problems. I'd just like to fix the worst offenders where we can.
There should be no change about which of the sources is preferred. The patcher will check the file server first. If the |
| hsLockGuard(fWorker->fRequestMut); | ||
| fWorker->fRequests.emplace_back("SecurePreloader", pfPatcherWorker::Request::kSecurePreloader); | ||
| if (NetCliFileQueryConnected()) { |
There was a problem hiding this comment.
what we're really interested in is whether or not a file server is available. If it isn't. then we should fall back to the auth server.
Ah, I see - that logic makes sense, and in that case I agree the new function is the better choice.
| if (NetCliFileQueryConnected()) { | ||
| fWorker->fRequests.emplace_back("SecurePreloader", pfPatcherWorker::Request::kSecurePreloader); | ||
| } else { | ||
| fWorker->EnqueuePreloaderLists(); | ||
| } |
There was a problem hiding this comment.
Regardless of how the condition is spelled, I'm still unsure if this behaves as intended in some corner cases.
Specifically: if /LocalData is specified, but not /LocalSDL, the intended behavior is that .sdl files are fetched from the server and everything else is taken from the local files (without patching) - correct?
This code works that way if the .sdl files are available from the auth server/secure preloader, but not if the .sdl files are only served from the file server. Currently, enabling /LocalData makes the client never connect to the file server, even if one exists - so in that case, this code will only check the auth server and not the file server for .sdl files. The fix would be to change all the conditions for connecting to the file server from if (!gDataServerLocal) to if (!gDataServerLocal || !gSDLLocal) I think.
This might be a purely theoretical problem - I'm not sure. All I know is that DIRTSAND-based servers usually provide the .sdl files via the file server, but I don't know if that also means that they're not available from the auth server anymore.
In any case, it's not a big concern. This fix is primarily meant for Cyan's shard, where .sdl files are available from the auth server, so the code will work correctly there.
There was a problem hiding this comment.
Specifically: if /LocalData is specified, but not /LocalSDL, the intended behavior is that .sdl files are fetched from the server and everything else is taken from the local files (without patching) - correct?
That's the way it's coded, yes. It could make more sense, in some ways, to only allow /LocalSDL if /LocalData is specified because using the shard's data server but your own bespoke SDL seems a little strange.
This code works that way if the .sdl files are available from the auth server/secure preloader, but not if the .sdl files are only served from the file server. Currently, enabling /LocalData makes the client never connect to the file server, even if one exists - so in that case, this code will only check the auth server and not the file server for .sdl files. The fix would be to change all the conditions for connecting to the file server from if (!gDataServerLocal) to if (!gDataServerLocal && !gSDLLocal) I think.
This might be a purely theoretical problem - I'm not sure. All I know is that DIRTSAND-based servers usually provide the .sdl files via the file server, but I don't know if that also means that they're not available from the auth server anymore.
In any case, it's not a big concern. This fix is primarily meant for Cyan's shard, where .sdl files are available from the auth server, so the code will work correctly there.
Yeah, we never actually wrote down any sort of guidelines about how Python and SDL should be served. In my mind, you really want to serve both Python and SDL from both locations - auth and file server. UruManifest will deploy Python and SDL for both auth and file servers, regardless of server software. To be honest, though, once you start playing around with /LocalData and especially /LocalSDL, you're explicitly turning off the safety valves and saying, "Yeah, I know what I'm doing. If things go badly, it's my fault." This PR is mostly to prevent bad things from happening to other people playing alongside you with some fringe benefits for people using H'uru clients on MOULa.
There was a problem hiding this comment.
In my mind, you really want to serve both Python and SDL from both locations - auth and file server.
Okay - if that's the preferred/common practice, then this code is fine.
(I made a small logic error above btw: if we were to change the file server connect conditions, they should become if (!gDataServerLocal || !gSDLLocal), with a || rather than && - but that doesn't really matter now.)
The impetus for this change is the SDL on MOULa is now progressing, sometimes faster than we can update it due to limited reviewer manpower and the lack of proper reviews taking place on the OU side. This results in SDL DESC PROBLEM for people using our clients. Further, it is known that the MOULa game server does not inspect SDL blobs before propagating them to other players, so it is fairly trivial to make clients error out on MOULa with cryptic messages by taking specially crafted SDLs on that shard. Therefore, we now prefer to use the server's SDL, even in /LocalData.
This removes the singlular
/SkipPreloadcommand line argument in favor of two arguments:/LocalPythonand/LocalSDL./LocalDatanow implies the former but not the latter. The reason for this change is that the SDL is a contract between the server and the client about how exactly an SDL blob is formatted. If SDL blobs don't have the same format between the client and server, this can have fairly bad implications, ranging from remote players being disconnected with an error, the player with the wrong SDL getting an error, or the server crashing.