Unable to create server

[ghost]

Hey,

I'm new to rust and I thought it would be a nice challenge to create some basic Minecraft server in Rust. Just with an implementation of authentication and chatting. However it failed at receiving a simple Handshake packet: 😄

use std::thread;
use std::time::Duration;
use ozelot::{Server};
use ozelot::serverbound::ServerboundPacket;

use std::net::{TcpListener, TcpStream};

fn main() -> std::io::Result<()> {

    let listener = TcpListener::bind("0.0.0.0:25565")?;

    for stream in listener.incoming() {
        handle_client(stream?);
    }
    Ok(())
}

fn handle_client(stream: TcpStream) {

    println!("new client");
    let mut server = Server::from_tcpstream(stream).unwrap();

    loop {
        let packets = server.read().unwrap();
        for packet in packets {
            match packet {
                ServerboundPacket::Handshake(ref _p) => {
                    println!("handshake request");
                    
                    break;
                },
                _ => (),
            }
        }
        thread::sleep(Duration::from_millis(50));
    }
}

Error when refreshing server list or connecting to the server:

new client
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error(Io(Custom { kind: UnexpectedEof, error: "failed to fill whole buffer" }), State { next_error: None, backtrace: InternalBacktrace { backtrace: Some(   0: backtrace::backtrace::trace_unsynchronized
   1: backtrace::backtrace::trace
   2: backtrace::capture::Backtrace::create
   3: backtrace::capture::Backtrace::new_unresolved
   4: error_chain::backtrace::imp::InternalBacktrace::new
   5: <error_chain::State as core::default::Default>::default
   6: ozelot::errors::Error::from_kind
   7: <ozelot::errors::Error as core::convert::From<std::io::error::Error>>::from
   8: ozelot::read::read_varint
   9: ozelot::serverbound::Handshake::deserialize
  10: <ozelot::serverbound::ServerboundPacket as ozelot::connection::Packet>::deserialize
  11: ozelot::connection::Connection<I,O>::read_packet
  12: ozelot::server::Server::read_packet
  13: ozelot::server::Server::read
  14: squid_grid::handle_client
  15: squid_grid::main
  16: std::rt::lang_start::{{closure}}
  17: std::panicking::try::do_call
  18: __rust_maybe_catch_panic
  19: std::rt::lang_start_internal
  20: std::rt::lang_start
  21: main
) } })', src/libcore/result.rs:1165:5

Maybe someone nice can help me out. 🙂

Best ImuustMINE

[ghost]

Yes, kind of. Instead of the server.read() method I am now using the server.read_packet() method. Here is some code: https://gist.github.com/ImuustMINE/0df1f1292b66c4b75575435791937fbf

But I am not sure if this approach is the most efficient one.

[C4K3]

The failed to fill whole buffer is likely an invalid packet definition in ozelot. (The error means ozelot expected to be able to read some bytes into some field in some packet, but the message was too short.)

It is possible it was just getting an invalid packet, but most likely it's a bug in ozelot.

One change I'd like to make (or have made :-) ) is to add an option for lazy deserialization. Right now ozelot will try to deserialize every packet, but for most uses we aren't interested in fully implementing the protocol so that's not necessary. Also it'd be nice to improve the error messages (and move off error-chain) so it'd be easier to narrow down where the bug is.

[thisjaiden]

Hello! I've been looking into this lately and have identified the source of the problem. When a new Server instance is created it defaults the internal ClientState to Handshake, which has only one packet with an ID of 0. When a client pings a server for the status and MOTD, it will send one packet in the Handshake state which immediately requests a change to the Status state. It follows up with another packet that has different fields but has the same ID of 0. Ozelot is trying to interpret both packets as a Handshake packet, and thus is having an error when one of them is instead a StatusPing packet.

This is the workaround I'm using:

use std::thread;
use std::time::Duration;
use ozelot::{
    Server,
    serverbound::ServerboundPacket,
    ClientState
};
use std::net::TcpListener;

static IP: &str = "0.0.0.0";
static PORT: &str = "223";

fn main() {
    // bind to external port to listen
    let listener = TcpListener::bind(format!("{}:{}", IP, PORT)).unwrap();
    // for every new connection...
    for stream in listener.incoming() {
        // TODO: these unwraps should be dealt with better
        let mut server_instance = Server::from_tcpstream(stream.unwrap()).unwrap();
        loop {
            // read new data from the network, if any
            server_instance.update_inbuf().unwrap();
            // check if we have a full packet
            let potential_packet = server_instance.read_packet().unwrap();
            if let Some(packet) = potential_packet {
                // evaluate the packet if we have one
                match packet {
                    ServerboundPacket::Handshake(data) => {
                        if data.get_next_state() == &1 {
                            // **THIS IS THE MAGIC THAT PREVENTS THE NASTY CRASH**
                            server_instance.set_clientstate(ClientState::Status);
                        }
                    }
                }
            }
            // wait a little bit before checking again
            thread::sleep(Duration::from_millis(50));
        }
    }
}

I think Server.read_packet() is probably fine as a primary api if update_inbuf() was integrated somehow, but this issue does make Server.read() pretty much useless considering there is no way to change the way it reads the packets if they arrive at the same time.

I see two solutions to this:

1. Change how ozelot decodes packets so that it automatically calls Server.set_clientstate() when appropriate
2. Change the API to focus on Server.read_packet() instead of Server.read()

I'd be happy to submit a PR with fixes for either of the soulutions, but I'd like to hear what @C4K3 thinks would be best.

All the best, - Jaiden