Received length loss

[wfeii1980]

This question comes from last branch main.

    fn handle_read(&mut self, ip: &IpAddr) -> bool {
        let intf_sock = match self.intf_socks.get_mut(ip) {
            Some(if_sock) => if_sock,
            None => return false,
        };
        let mut buf = vec![0u8; MAX_MSG_ABSOLUTE];
        let sz = match intf_sock.sock.read(&mut buf) {
            Ok(sz) => sz,
            Err(e) => {
                if e.kind() != std::io::ErrorKind::WouldBlock {
                    error!("listening socket read failed: {}", e);
                }
                return false;
            }
        };

        debug!("received {} bytes", sz);

        // If sz is 0, it means sock reached End-of-File.
        if sz == 0 {
            error!("socket {:?} was likely shutdown", intf_sock);
            if let Err(e) = self.poller.delete(&intf_sock.sock) {
                error!("failed to remove sock {:?} from poller: {}", intf_sock, &e);
            }

            // Replace the closed socket with a new one.
            match new_socket_bind(&intf_sock.intf) {
                Ok(sock) => {
                    let intf = intf_sock.intf.clone();
                    self.intf_socks.insert(*ip, IntfSock { intf, sock });
                    debug!("reset socket for IP {}", ip);
                }
                Err(e) => error!("re-bind a socket to {}: {}", ip, e),
            }
            return false;
        }

        // ******************** buf.len() == MAX_MSG_ABSOLUTE != sz
        match DnsIncoming::new(buf) {
            Ok(msg) => {
                if msg.is_query() {
                    self.handle_query(msg, ip);
                } else if msg.is_response() {
                    self.handle_response(msg);
                } else {
                    error!("Invalid message: not query and not response");
                }
            }
            Err(e) => error!("Invalid incoming DNS message: {}", e),
        }

        true
    }

[wfeii1980]

buf.set_len(sz) is unsafe
buf.truncate(sz) is ok

[wfeii1980]

Two panic!

        for _ in 0..n {
            let name = self.read_name()?;
            let slice = &self.data[self.offset..];
            // ***************** slice.len() < 10
            let ty = u16_from_be_slice(&slice[..2]);
            let class = u16_from_be_slice(&slice[2..4]);
            let ttl = u32_from_be_slice(&slice[4..8]);
            let length = u16_from_be_slice(&slice[8..10]) as usize;
            self.offset += 10;
            let next_offset = self.offset + length;

            // Check the first 2 bits for possible "Message compression".
            match length & 0xC0 {
                0x00 => {
                    // regular utf8 string with length
                    offset += 1;
                    // **************** (offset + length as usize) > data.len()
                    name += str::from_utf8(&data[offset..(offset + length as usize)])
                        .map_err(|e| Error::Msg(format!("read_name: from_utf8: {}", e)))?;
                    name += ".";
                    offset += length as usize;
                }

[wfeii1980]

In my environment, MDNS has a relatively large amount of data, and the problem of data loss is often caused by the read cache being full. So it also causes the above problems.

[keepsimple1]

buf.set_len(sz) is unsafe buf.truncate(sz) is ok

I couldn't remember why we didn't truncate the buffer after the read. One possible reason is DnsIncoming decoding the buffer does not require the truncate (The decoding relies on the message format). But anyhow we can add buf.truncate(sz).

[keepsimple1]

// ***************** slice.len() < 10

I think it's a good idea to add a check for the slice length and return Err if fails.

[keepsimple1]

// **************** (offset + length as usize) > data.len()

Same here, I agree it's good to check the length here.

[keepsimple1]

In my environment, MDNS has a relatively large amount of data, and the problem of data loss is often caused by the read cache being full. So it also causes the above problems.

What the typical and max size of your mDNS message? Did you mean the mDNS message in your env. is larger than MAX_MSG_ABSOLUTE i.e. 8966 bytes? The RFC says a Multicast DNS packet, including IP and UDP headers, MUST NOT exceed 9000 bytes (here).

[keepsimple1]

For the sanity checks, I've opened a PR to add them. Let me know if you have any questions or comments.

[keepsimple1]

PR merged. Let me know if you have any questions.

[wfeii1980]

In my environment, MDNS has a relatively large amount of data, and the problem of data loss is often caused by the read cache being full. So it also causes the above problems.

What the typical and max size of your mDNS message? Did you mean the mDNS message in your env. is larger than MAX_MSG_ABSOLUTE i.e. 8966 bytes? The RFC says a Multicast DNS packet, including IP and UDP headers, MUST NOT exceed 9000 bytes (here).

I did receive an oversized package, if it's illegal, then ignore it.

mdns-big.zip

[wfeii1980]

PR merged. Let me know if you have any questions.

Good!

[keepsimple1]

In my environment, MDNS has a relatively large amount of data, and the problem of data loss is often caused by the read cache being full. So it also causes the above problems.

What the typical and max size of your mDNS message? Did you mean the mDNS message in your env. is larger than MAX_MSG_ABSOLUTE i.e. 8966 bytes? The RFC says a Multicast DNS packet, including IP and UDP headers, MUST NOT exceed 9000 bytes (here).

I did receive an oversized package, if it's illegal, then ignore it.

mdns-big.zip

According to WireShark, the packet size is 1030 bytes, far from the max size (8966 bytes). I didn't see obvious problems here. Let me know if you encountered any particular issues.

[wfeii1980]

All 11 frames form one package.

[keepsimple1]

Oh I missed that, sorry. It is a fragmented IP packet. We don't support fragmented IP packets yet. This would be a new feature and it will take some time to implement if I would do it. Would you be interested in creating a PR to add this new feature to support IP fragments?

[dalepsmith]

Basically, an mdns datagram can not be larger than 9000 even when fragmented into multiple UDP packets. And also can not contain more than one resource record. From the mdns rfc rfc6761: 17. Multicast DNS Message Size The 1987 DNS specification [RFC1035] restricts DNS messages carried by UDP to no more than 512 bytes (not counting the IP or UDP headers). For UDP packets carried over the wide-area Internet in 1987, this was appropriate. For link-local multicast packets on today's networks, there is no reason to retain this restriction. Given that the packets are by definition link-local, there are no Path MTU issues to consider. Multicast DNS messages carried by UDP may be up to the IP MTU of the physical interface, less the space required for the IP header (20 bytes for IPv4; 40 bytes for IPv6) and the UDP header (8 bytes). In the case of a single Multicast DNS resource record that is too large to fit in a single MTU-sized multicast response packet, a Multicast DNS responder SHOULD send the resource record alone, in a single IP datagram, using multiple IP fragments. Resource records this large SHOULD be avoided, except in the very rare cases where they really are the appropriate solution to the problem at hand. Implementers should be aware that many simple devices do not reassemble fragmented IP datagrams, so large resource records SHOULD NOT be used except in specialized cases where the implementer knows that all receivers implement reassembly, or where the large resource record contains optional data which is not essential for correct operation of the client. A Multicast DNS packet larger than the interface MTU, which is sent using fragments, MUST NOT contain more than one resource record. Even when fragmentation is used, a Multicast DNS packet, including IP and UDP headers, MUST NOT exceed 9000 bytes. Note that 9000 bytes is also the maximum payload size of an Ethernet "Jumbo" packet [Jumbo]. However, in practice Ethernet "Jumbo" packets are not widely used, so it is advantageous to keep packets under 1500 bytes whenever possible. Even on hosts that normally handle Ethernet "Jumbo" packets and IP fragment reassembly, it is becoming more common for these hosts to implement power-saving modes where the main CPU goes to sleep and hands off packet reception tasks to a more limited processor in the network interface hardware, which may not support Ethernet "Jumbo" packets or IP fragment reassembly.

…

-Dale

[keepsimple1]

Regarding IP fragmentation, it seems that UDP packet received by the socket should already be reassembled, if no dropped packets. From ChatGPT:
In summary, IP handles fragmentation and reassembly, and this applies to both TCP and UDP traffic. UDP itself, being a simple and connectionless protocol, doesn't have built-in mechanisms for handling fragmentation or reassembly.
That means we need not (and should not) have special logic to handle IP fragments.

And as what Dale mentioned :

Basically, an mdns datagram can not be larger than 9000 even when
fragmented into multiple UDP packets. And also can not contain more
than one resource record.

We should be good with the current max size (around 9000). For any large RR (resource record), it should be sent in a separate mDNS datagram.

Looking back at your capture, it seems that all 377 RRs are sent in a single datagram of 15794 bytes, which conflicts the RFC recommendation.

[dalepsmith]

I'm curious what is generating that *huge* datagram! Is that Bonjour or Avahi? Some other library? Something homebrewed? What kind of device is sending that? Thanks! -Dale

…

On 2/5/24, keepsimple1 ***@***.***> wrote: Regarding IP fragmentation, it seems that UDP packet received by the socket should already be reassembled, if no dropped packets. From ChatGPT: ``` In summary, IP handles fragmentation and reassembly, and this applies to both TCP and UDP traffic. UDP itself, being a simple and connectionless protocol, doesn't have built-in mechanisms for handling fragmentation or reassembly. ``` That means we need not (and should not) have special logic to handle IP fragments. And as what Dale mentioned : > Basically, an mdns datagram can not be larger than 9000 even when > fragmented into multiple UDP packets. And also can not contain more > than one resource record. We should be good with the current max size (around 9000). For any large RR (resource record), it should be sent in a separate mDNS datagram. Looking back at your capture, it seems that all 377 RRs are sent in a single datagram of 15794 bytes, which conflicts the RFC recommendation. <img width="1430" alt="mdns-wireshark-large" src="https://github.com/keepsimple1/mdns-sd/assets/7699244/cdd2e5d1-ed7e-4d84-8654-68581b5c6ac5"> -- Reply to this email directly or view it on GitHub: #168 (comment) You are receiving this because you commented. Message ID: ***@***.***>

[wfeii1980]

I'm curious what is generating that huge datagram! Is that Bonjour or Avahi? Some other library? Something homebrewed? What kind of device is sending that? Thanks! -Dale

I guess it comes from the jmdns package used by my colleague, but I haven't personally verified it yet.

[keepsimple1]

I don't think we need to support such datagram that is clearly outside of the RFC. On the other hand, I will think of how we can handle such cases more gracefully.

[keepsimple1]

I had to re-learn some details about UDP socket API. Here is a related info about the recv/recv_from API on Linux:

If a message is too long to fit in the supplied buffer, excess bytes may be discarded depending on the type of socket the message is received from.

and in the classic book "TCP/IP Illustrated, volume 1", chapter 11, section 11.10 Maximum UDP Datagram Size, it says this about "Datagram Truncation":

What happens if the received datagram exceeds the size the application is prepared to deal with? Unfortunately the answer depends on the programming interface and the implementation.

It then says the sockets API under SVR4 Unix does not truncate the datagram.

Based on the above, I think there is no much more we can do (or should do) besides the sanity checks in the current code.

If a mDNS datagram happens to exceed the max buffer size, it will not be decoded correctly / fully. And if there is no truncation, the follow-up read will have invalid headers, etc, and will fail the decode process with an Err return. As long as there is no crash, I think we are good.

[keepsimple1]

I've merged in the minor change of MAX_MSG_ABSOLUTE and added comments. Will close this issue. If you have any additional questions, please re-open this issue or open a new issue. Cheers!