BBA/BuiltIn: Add UPnP HTTP listener #10985
Conversation
|
Would it be possible to create automated testing like what fifoCI does for graphics? Each change affects several games and has the chance to introduce regressions. |
This would be quite complicated. We already have UnitTest test cases to ensure some features of Dolphin are working properly. I could write hardware tests (homebrew) to check some networking behaviours like I did for small Wii networking edge cases. The main issue with complex cases like this one would be the reliability. Timing might alter the test results, I don't think TASing would work since the network behaviour isn't very deterministic. Overall, I don't think that's easily feasible as of yet. |
|
I've tested the PR and here are the findings from my side: -The -By adding a breakpoint into the I might not be too savvy with networking security but I think its a security issue opening a port and setting it up to receive/send crafted data like this xml like a pseudo small web server service. Maybe there's another way without resorting to this method? PD: On another note, I really suggest you procure the PSO Episode 1 & 2 iso and access the online servers if you will keep making these changes and test it live see if something breaks, it will give you a valuable information as you keep changing stuff in the BBA BuiltIn stack. |
|
@nolrinale Opening a port shouldn't be a security issue per se, even if it does increase the network attack surface. Unless the game is vulnerable to remote code execution attacks from its network stack, this shouldn't be an issue here as we only forward the network traffic to the BBA code. If such vulnerability exists, then you found a RCE for MKDD and Kirby Air Ride (which would be nice for homebrew related stuff on non-modded GameCube). If this doesn't fix the PSO warning, then the error is located somewhere else. I just want to make sure this PR (which improve the BBA emulation accuracy for other games) doesn't break existing features as I don't own all BBA games. Importing items in my country is a pain and not cheap either, if I find the opportunity to grab some of these games I will but that's clearly not my priority, at the moment. |
|
@sepalani I can test this build today on windows 10 x64 for a few hours. Is there something specific I should do to test your changes? Like, run pr-win-x64 build with specific settings or running the pr-win-dbg-x64 build? |
|
I"ve been testing the pr-win-x64 build for a few hours and haven't disconnected. Seems to be stable for my build. |
|
Everything seems to be working as expected, but does need that other PR. I think we just need a review on these. |
| @@ -727,6 +779,22 @@ sf::Socket::Status BbaTcpSocket::Connect(const sf::IpAddress& dest, u16 port, u3 | |||
| return this->connect(dest, port); | |||
| } | |||
|
|
|||
| sf::Socket::Status BbaTcpSocket::GetPeerName(sockaddr_in* addr) | |||
There was a problem hiding this comment.
This isn't a lot of code...but maybe add a helper function that both implementations call? This seems like the same implementation
There was a problem hiding this comment.
They are slightly similar but serve different purposes just like C socket's getpeername and getsockname. That's why I don't think its a good idea to merge them into a single one. The reason I created a wrapper for them is to have access to the SFML Socket protected member function getHandle.
| { | ||
| if (ref == nullptr) | ||
| return; // not found | ||
|
|
||
| ref->ack_num++; | ||
| ref->ack_num += 1 + static_cast<u32>(data.size()); |
There was a problem hiding this comment.
All the other ack_num values being set use ntohl did you intentionally leave it off here for some reason?
There was a problem hiding this comment.
ack_num is using the host's endianness and is used to acknowledge the TCP packet bytes (sequence number) received. When inspecting TCP packets from the BBA buffer we need to convert from network to host (ntoh*) endianness. Here we're just incrementing it (not setting it from a network packet) so we don't need to swap it.
|
@iwubcode I suspect Dolphin's HLE BBA to have similar limitations with Nintendont HLE BBA. I wonder if it's related to the MAC addresses not being valid BBA ones. |
|
After some testing, it seems to behave just like before the rebase. I also found the issue I had with one of my PC and its VM: |
|
I inverted the first |
|
I'm surprised this doesn't conflict with 10920? Is this good to merge as well? |
I'm surprised too, I did a rebase just for good measure. Though, I'm surprised that GitHub was still showing 4 commits to be merged instead of one. I'm also done with this PR. |
This PR implements the UPnP HTTP listener used by Kirby Air Ride and Mario Kart Double Dash. If someone on the same network of these GC games visits the URL
http://<GameCube_IP>:1900/rootDesc.xml, they can grab the UPnP XML file. I confirmed this behaviour using Nintendont on a real Wii (since I don't have a BBA adapter). This URL can only be accessed from another IP (i.e. an IP other than the one used by Dolphin).@schthack @nolrinale
This might also address this issue https://dolp.in/i12996 as this PR fixes an issue where partial data isn't sent when closing the connection. I touched the TCP emulation part so you might want to test this out to make sure it doesn't impact PSO servers.
This PR doesn't fix MKDD timer stuck at 172 when there are more than 2 players. Depends on #10920.