Fix issue #10 and a bunch of other stuff#11
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #10 (random connection/ping failures) by fixing multiple critical bugs and implementing architectural improvements to prevent connection stalls in the mineginx Minecraft proxy server.
Changes:
- Fixed multiple critical bugs in packet parsing (buffer expansion panic, off-by-one errors, packet length calculations, string UTF-8 handling)
- Replaced complex oneshot channel signaling with simpler TCP FIN-based coordination for bidirectional stream forwarding
- Pre-resolved DNS at startup to prevent blocking on musl/scratch containers, with lazy resolution fallback for upstreams unavailable at startup
- Upgraded TCP listener with socket2 for larger connection backlog (1024) and SO_REUSEADDR support
- Improved error handling by replacing
.unwrap()calls and adding global panic hook for diagnostics - Enhanced domain matching with case-insensitive comparison and trailing dot normalization
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| mineginx/src/stream.rs | Simplified bidirectional TCP forwarding by removing oneshot channels and using TCP FIN for coordination; added comprehensive documentation |
| mineginx/src/main.rs | Implemented DNS pre-resolution with lazy fallback, socket2 integration for robust listener setup, panic hook for diagnostics, and improved error handling |
| minecraft/src/serialization.rs | Fixed critical bugs: implemented buffer expansion (was todo!()), corrected off-by-one in data_len(), added varint_size() for accurate packet length calculation, improved string reader with proper bounds checking and UTF-8 error handling |
| mineginx/Cargo.toml | Added socket2 dependency for advanced socket configuration |
| Cargo.lock | Updated dependencies including socket2 0.5.10 and transitive Windows dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -191,6 +241,12 @@ const CONFIG_FILE: &str = "./config/mineginx.yaml"; | |||
| #[tokio::main(flavor = "multi_thread")] | |||
| async fn main() -> ExitCode { | |||
| SimpleLogger::new().init().unwrap(); | |||
There was a problem hiding this comment.
The .unwrap() call on logger initialization should be replaced with proper error handling. If logger initialization fails, the application should either handle it gracefully or exit with a clear error message. This is especially important given that the PR description mentions replacing .unwrap() calls with error handling.
| value >>= 7; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The new varint_size function calculates the encoded size of a VarInt, which is critical for the packet length fix. However, there are no tests verifying this function works correctly. Given that the minecraft crate has existing test infrastructure (tests/field_types.rs, tests/truncate_to_zero.rs), consider adding test cases that verify varint_size returns the correct values for edge cases like 0, 127, 128, -1, and maximum/minimum i32 values. This would help prevent regressions in this critical protocol parsing logic.
| #[cfg(test)] | |
| mod tests { | |
| use super::varint_size; | |
| #[test] | |
| fn test_varint_size_edge_cases() { | |
| // Basic small values | |
| assert_eq!(varint_size(0), 1); | |
| assert_eq!(varint_size(1), 1); | |
| assert_eq!(varint_size(127), 1); | |
| // Boundary where an extra byte is needed | |
| assert_eq!(varint_size(128), 2); | |
| // Negative values and extremes of i32 range | |
| assert_eq!(varint_size(-1), 5); | |
| assert_eq!(varint_size(i32::MAX), 5); | |
| assert_eq!(varint_size(i32::MIN), 5); | |
| } | |
| } |
| fn expand_buffer(&mut self) { | ||
| todo!() | ||
| let new_len = self.buffer.len() * 2; | ||
| self.buffer.resize(new_len, 0); | ||
| } |
There was a problem hiding this comment.
The buffer expansion logic was changed from todo!() to actual implementation, and the data_len() calculation had an off-by-one error fixed. Given that the minecraft crate has existing test infrastructure, consider adding tests that verify buffer expansion works correctly when the buffer fills up, and that data_len() returns accurate values in various states (empty buffer, partial data, full buffer). These are critical bugs that were fixed and should have regression tests.
| } | ||
| let packet = match MinecraftPacket::make_raw(0, &handshake) { | ||
| Some(v) => v, | ||
| None => return |
There was a problem hiding this comment.
Early return without error logging is inconsistent with the error handling pattern used elsewhere in this function (e.g., lines 123, 128). Consider logging an error message before returning to aid debugging, especially since packet serialization failures could indicate protocol issues.
| None => return | |
| None => { | |
| error!("failed to serialize handshake packet for upstream: {}", upstream.proxy_pass); | |
| return; | |
| } |
| match upstream.write_all(&packet[0..packet.len()]).await { | ||
| match upstream_conn.write_all(&packet[0..packet.len()]).await { | ||
| Ok(_) => { }, | ||
| Err(_) => return |
There was a problem hiding this comment.
Early return without error logging is inconsistent with the error handling pattern used elsewhere in this function. Consider logging an error message to indicate that writing the handshake packet to the upstream failed, which would aid in diagnosing connection issues.
| Err(_) => return | |
| Err(e) => { | |
| error!("failed to write handshake packet to upstream '{}': {}", upstream.proxy_pass, e); | |
| return; | |
| } |
| error!("failed to listen on {}: {e}", &server.listen); | ||
| return ExitCode::from(3); | ||
| } | ||
| socket.set_nonblocking(true).unwrap(); |
There was a problem hiding this comment.
The .unwrap() call on set_nonblocking(true) should be replaced with proper error handling like the other socket operations in this section. This is inconsistent with the surrounding error handling pattern where all socket operations return errors via ExitCode::from(3).
mineginx/src/main.rs
Outdated
| cache.insert(server_desc.proxy_pass.clone(), entry.clone()); | ||
| entry |
There was a problem hiding this comment.
There's a potential race condition in the lazy DNS resolution logic. If multiple clients connect simultaneously to the same unresolved upstream, they will all attempt to resolve it concurrently (line 97). While each will successfully insert their result into the cache (line 106), this results in unnecessary duplicate DNS lookups. Consider using a more sophisticated caching strategy like checking again after acquiring the write lock, or using a Mutex around the resolution attempt itself to ensure only one resolution happens per upstream.
| cache.insert(server_desc.proxy_pass.clone(), entry.clone()); | |
| entry | |
| // Re-check the cache under the write lock in case another task | |
| // has already resolved and inserted this upstream. | |
| if let Some(existing) = cache.get(&server_desc.proxy_pass).cloned() { | |
| existing | |
| } else { | |
| cache.insert(server_desc.proxy_pass.clone(), entry.clone()); | |
| entry | |
| } |
| match upstream.write_all(&minecraft.take_buffer()).await { | ||
| match upstream_conn.write_all(&minecraft.take_buffer()).await { | ||
| Ok(_) => {}, | ||
| Err(_) => { |
There was a problem hiding this comment.
Early return without error logging is inconsistent with the error handling pattern used elsewhere in this function. Consider logging an error message to indicate that flushing the unread buffer to the upstream failed.
| Err(_) => { | |
| Err(e) => { | |
| error!("failed to flush unread buffer to upstream: {}", e); |
| None => return | ||
| }; | ||
| match upstream.write_all(&packet[0..packet.len()]).await { | ||
| match upstream_conn.write_all(&packet[0..packet.len()]).await { |
There was a problem hiding this comment.
The slice &packet[0..packet.len()] is redundant and can be simplified to &packet since it represents the entire slice. This is a minor style improvement for code readability.
| match upstream_conn.write_all(&packet[0..packet.len()]).await { | |
| match upstream_conn.write_all(&packet).await { |
Folleach
left a comment
Folleach
left a comment
There was a problem hiding this comment.
Wow, that is so many changes you've made! Hello!
Most of these improvements are spot on and really make sense.
I grateful how you rewrote forward_stream function, it was awfuuuul, haha. I wrote it one evening after falling down a rabbit hole and told myself: "Will rewrite this someday", and now you've simplified it so much. Thanks a lot!
I've had a quick look at the code, and there are a few bits I'd like to tweak. If you could make those changes, that'd be awesome. Or I can handle it myself later when I have some free time.
Either way, this is a great improvements to the code base!
|
@Folleach I removed the DNS cache as it really was only there as a test to see if DNS resolution under docker was causing some weird issue with the pinging problems I was having. I also removed all the redundant comments and simplified the huge comment |
|
Looks great. |
Here is a bunch of changes that fix #10 and also fixes a random crash that happens. Also improve the code base to be way more robust.
I was sent down a rabbit hole because of the fact I forgot to up the file descriptor limit on my proxy server that sits between cloudflare and mineginx, thus causing connection limits
List of changes so far:
todo!()→ actual implementation).unwrap()calls in main with error handling